linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com>
To: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc: "r.verdejo@samsung.com" <r.verdejo@samsung.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"nicolas@ndufresne.ca" <nicolas@ndufresne.ca>,
	"linux-kernel-mentees@lists.linuxfoundation.org"
	<linux-kernel-mentees@lists.linuxfoundation.org>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>
Subject: Re: [Linux-kernel-mentees] [v10 0/4] media: vidtv: Implement a virtual DVB driver
Date: Sat, 12 Sep 2020 11:49:01 -0300	[thread overview]
Message-ID: <15028D87-A496-4CAD-91A6-E4467489C4D0@getmailspring.com> (raw)
In-Reply-To: <20200912111506.2d2bd512@coco.lan>

Hi Hans and Mauro & all


>Why the dvb_ prefix? All virtual drivers just start with 'vi'.
>
>And wouldn't it make more sense to call dvb_vidtv_bridge.ko just vidtv.ko?
>Just like the other virtual media drivers?

I guess Mauro was the one to come up with the dvb_* prefix for the
kernel modules for the reasons he explicited up in this thread. 

As far as dvb_vidtv_bridge.ko and vidtv_bridge.c, I just wanted to be
verbose so that people would look at this and see that it is the code
for a bridge driver, since this is also supposed to be a template. 

Also because I had some trouble myself figuring out what was what when
first browsing through other dvb drivers. That said, I am 100% onboard
with renaming this to vidtv.ko or whatever seems more appropiate :)


>Yet, I agree with you that at least an alias is needed.
>earlier today, I wrote a patch with such purpose:

If you all would like to just leave this at that ^ I am also ok with it.

>For regression testing of vidtv during the daily build it would be
great if
>the contrib/test/test-media script can be enhanced to include vidtv.

Sure, I can do that if you'd like. Can you provide some tips on how to
get started? :)

>Note that this script relies on the /dev/mediaX devices to run the
tests. I
>noticed that vidtv doesn't appear to create a /dev/mediaX device, even though
>CONFIG_MEDIA_CONTROLLER_DVB=y. This is definitely something that would
be good
>to support in vidtv.

I didn't write any code for this specifically. I was under the
impression that the dvb core would create any/all the necessary files. Mauro?


>I'm thinking on something like:
>
>echo 1 >/sys/kernel/debug/vidtv/discontinuity
>
>to generate a frame numbering discontinuity
>
>and other things like that, changing S/N ratio and other parameters
>and injecting errors, in order to test the effects on user apps.

Can you provide an initial list of things you would like to see? I can
then implement these and we can go from there

When you say 'frame numbering discontinuity', do you mean having a gap
in the the TS continuity counter for a given pid?

For the s302m encoder, maybe we can add some noise to the samples?

> changing S/N ratio

Btw 'lose lock if signal goes bad' code in vidtv_tuner.c and
vidtv_demod.c does not work currently. Mainly because they expect an
array of valid frequencies to compare to and as of now there's no default.

I mean these, just to be clear:

static unsigned int vidtv_valid_dvb_t_freqs[NUM_VALID_TUNER_FREQS];
module_param_array(vidtv_valid_dvb_t_freqs, uint, NULL, 0);
MODULE_PARM_DESC(vidtv_valid_dvb_t_freqs,
  		 "Valid DVB-T frequencies to simulate");

static unsigned int vidtv_valid_dvb_c_freqs[NUM_VALID_TUNER_FREQS];
module_param_array(vidtv_valid_dvb_c_freqs, uint, NULL, 0);
MODULE_PARM_DESC(vidtv_valid_dvb_c_freqs,
		 "Valid DVB-C frequencies to simulate");

static unsigned int vidtv_valid_dvb_s_freqs[NUM_VALID_TUNER_FREQS];
module_param_array(vidtv_valid_dvb_s_freqs, uint, NULL, 0);
MODULE_PARM_DESC(vidtv_valid_dvb_s_freqs,
		 "Valid DVB-C frequencies to simulate");

static unsigned int max_frequency_shift_hz;
module_param(max_frequency_shift_hz, uint, 0);
MODULE_PARM_DESC(max_frequency_shift_hz,
		 "Maximum shift in HZ allowed when tuning in a channel")






> No. I meant the audio was not a sinusoidal wave after such change.

This encoder is dead simple and yet I have been struggling :/

Here is the relevant decoding code in libavcodec/s302m.c:

        for (; buf_size > 4; buf_size -= 5) {
            *o++ = (ff_reverse[buf[1]]        <<  8) |
                    ff_reverse[buf[0]];
            *o++ = (ff_reverse[buf[4] & 0xf0] << 12) |
                   (ff_reverse[buf[3]]        <<  4) |
                   (ff_reverse[buf[2]]        >>  4);
            buf += 5;
        }

I am somewhat confident that the opposite to the above is precisely what
I sent you last:

> f.data[0] = reverse[sample & 0xff];
> f.data[1] = reverse[(sample & 0xff00) >> 8];
> f.data[2] = (vucf << 4) | (reverse[(sample & 0x0f)] >> 4);
> f.data[3] = reverse[(sample & 0x0ff0) >> 4];
> f.data[4] = reverse[(sample & 0xf000) >> 12] >> 4;


Their encoder, which works perfectly of course, is:

libavcodec/s302menc.c:

        for (c = 0; c < frame->nb_samples; c++) {
            uint8_t vucf = s->framing_index == 0 ? 0x10 : 0;

            for (channels = 0; channels < avctx->channels; channels +=
2) {
                o[0] = ff_reverse[ samples[0] & 0xFF];
                o[1] = ff_reverse[(samples[0] & 0xFF00) >>  8];
                o[2] = ff_reverse[(samples[1] & 0x0F)   <<  4] | vucf;
                o[3] = ff_reverse[(samples[1] & 0x0FF0) >>  4];
                o[4] = ff_reverse[(samples[1] & 0xF000) >> 12];
                o += 5;
                samples += 2;

            }

            s->framing_index++;
            if (s->framing_index >= 192)
                s->framing_index = 0;
        }

In which samples[0] and samples[1] are the same for stereo audio.

If you feel like we should only address this later I can drop this
subject but as we've both established, the encoder in v10 is flat out
wrong. Your call :)


-- thanks
-- Daniel

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

  reply	other threads:[~2020-09-12 14:49 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-21 12:58 [Linux-kernel-mentees] [v10 0/4] media: vidtv: Implement a virtual DVB driver Daniel W. S. Almeida
2020-08-21 12:58 ` [Linux-kernel-mentees] [v10 1/4] media: vidtv: implement a tuner driver Daniel W. S. Almeida
2020-08-21 12:58 ` [Linux-kernel-mentees] [v10 2/4] media: vidtv: implement a demodulator driver Daniel W. S. Almeida
2020-08-21 12:58 ` [Linux-kernel-mentees] [v10 3/4] media: vidtv: add a bridge driver Daniel W. S. Almeida
2020-09-15 11:53   ` Geert Uytterhoeven
2020-09-15 13:26     ` Daniel W. S. Almeida
2020-09-15 13:35       ` Geert Uytterhoeven
2020-09-15 18:13         ` Daniel W. S. Almeida
2020-09-16  7:01         ` Mauro Carvalho Chehab
2020-08-21 12:58 ` [Linux-kernel-mentees] [v10 4/4] media: Documentation: vidtv: Add ReST documentation for vidtv Daniel W. S. Almeida
2020-09-11  8:02 ` [Linux-kernel-mentees] [v10 0/4] media: vidtv: Implement a virtual DVB driver Mauro Carvalho Chehab
2020-09-11 12:18   ` Daniel W. S. Almeida
2020-09-11 13:10     ` Mauro Carvalho Chehab
2020-09-11 13:56       ` Mauro Carvalho Chehab
2020-09-12  2:54         ` Daniel W. S. Almeida
2020-09-12  7:38           ` Mauro Carvalho Chehab
2020-09-12  8:21 ` Hans Verkuil
2020-09-12  9:15   ` Mauro Carvalho Chehab
2020-09-12 14:49     ` Daniel W. S. Almeida [this message]
2020-09-12 17:57       ` Mauro Carvalho Chehab
2020-09-14  8:33         ` Hans Verkuil
2020-09-12  8:35 ` Hans Verkuil

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=15028D87-A496-4CAD-91A6-E4467489C4D0@getmailspring.com \
    --to=dwlsalmeida@gmail.com \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab+huawei@kernel.org \
    --cc=nicolas@ndufresne.ca \
    --cc=r.verdejo@samsung.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).