Re: [RFC, WIP, v4 07/11] media: vidtv: add MPEG TS common code

[Daniel W. S. Almeida]

Hi Mauro, thanks for reviewing this!

> Em Sat,  2 May 2020 00:22:12 -0300
> "Daniel W. S. Almeida" <dwlsalmeida@xxxxxxxxx> escreveu:
>
>> From: "Daniel W. S. Almeida" <dwlsalmeida@xxxxxxxxx>
>>
>> Add code to work with MPEG TS packets, such as TS headers, adaptation
>> fields, PCR packets and NULL packets.
>>
>> Signed-off-by: Daniel W. S. Almeida <dwlsalmeida@xxxxxxxxx>
>> ---
>>   drivers/media/test-drivers/vidtv/Makefile   |   2 +-
>>   drivers/media/test-drivers/vidtv/vidtv_ts.c | 130 ++++++++++++++++++++
>>   drivers/media/test-drivers/vidtv/vidtv_ts.h | 103 ++++++++++++++++
>>   3 files changed, 234 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/media/test-drivers/vidtv/vidtv_ts.c
>>   create mode 100644 drivers/media/test-drivers/vidtv/vidtv_ts.h
>>
>> diff --git a/drivers/media/test-drivers/vidtv/Makefile 
b/drivers/media/test-drivers/vidtv/Makefile
>> index 9ea9485d42189..92001bc348615 100644
>> --- a/drivers/media/test-drivers/vidtv/Makefile
>> +++ b/drivers/media/test-drivers/vidtv/Makefile
>> @@ -1,6 +1,6 @@
>>   # SPDX-License-Identifier: GPL-2.0
>>
>>   vidtv_demod-objs := vidtv_common.o
>> -vidtv_bridge-objs := vidtv_common.o
>> +vidtv_bridge-objs := vidtv_common.o vidtv_ts.o
>>
>>   obj-$(CONFIG_DVB_VIDTV)	+= vidtv_tuner.o vidtv_demod.o vidtv_bridge.o
>> diff --git a/drivers/media/test-drivers/vidtv/vidtv_ts.c 
b/drivers/media/test-drivers/vidtv/vidtv_ts.c
>> new file mode 100644
>> index 0000000000000..f545c45c0fe7c
>> --- /dev/null
>> +++ b/drivers/media/test-drivers/vidtv/vidtv_ts.c
>> @@ -0,0 +1,130 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * The Virtual DVB test driver serves as a reference DVB driver and 
helps
>> + * validate the existing APIs in the media subsystem. It can also aid
>> + * developers working on userspace applications.
>> + *
>> + * Written by Daniel W. S. Almeida <dwlsalmeida@xxxxxxxxx>
>> + */
>> +
>> +#include <linux/types.h>
>> +#include <asm/byteorder.h>
>> +#include "vidtv_ts.h"
>> +#include "vidtv_common.h"
>> +
>> +static u32 vidtv_ts_write_pcr_bits(u8 *buf, u64 pcr)
>> +{
>> +	/* Exact same from ffmpeg. PCR is a counter driven by a 27Mhz clock */
>> +	u64 pcr_low = pcr % 300, pcr_high = pcr / 300;
>> +
>> +	*buf++ = pcr_high >> 25;
>> +	*buf++ = pcr_high >> 17;
>> +	*buf++ = pcr_high >>  9;
>> +	*buf++ = pcr_high >>  1;
>> +	*buf++ = pcr_high <<  7 | pcr_low >> 8 | 0x7e;
>> +	*buf++ = pcr_low;
>> +
>> +	return 6;
>> +}
>> +
>> +void vidtv_ts_inc_cc(u8 *continuity_counter)
>> +{
>> +	++*continuity_counter;
>> +	if (*continuity_counter > TS_CC_MAX_VAL)
>> +		*continuity_counter = 0;
>> +}
>> +
>> +u32 vidtv_ts_null_write_into(struct null_packet_write_args args)
>> +{
>> +	u32    nbytes                  = 0;
>> +	struct vidtv_mpeg_ts ts_header = {0};
>> +
>> +	ts_header.sync_byte          = TS_SYNC_BYTE;
>> +	ts_header.pid                = TS_NULL_PACKET_PID;
>> +	ts_header.payload            = 1;
>> +	ts_header.continuity_counter = *args.continuity_counter;
>> +
>> +	cpu_to_be16s(&ts_header.bitfield);
>> +
>> +	/* copy TS header */
>> +	nbytes += vidtv_memcpy(args.dest_buf + args.dest_offset + nbytes,
>> +			       &ts_header,
>> +			       sizeof(ts_header),
>> +			       args.dest_offset + nbytes,
>> +			       args.buf_sz);
>
> Hmm... now I see why you're returning 0 to vidtv_memcpy().
>
> Yet, if the buffer is full, you should just drop the entire package,
> as otherwise you may end copying things that aren't multiple of 188 
bytes,
> causing sync issues at the client.

I'd like to provide a counterargument for this.

The way I am dealing with errors throughout vidtv so far is:
If we hit any of these WARN_ON macros, pr_err and the like, then all 
bets are off. This means that the resulting stream will likely be 
invalid and that something needs to be rewritten in the source code and 
my main concern is then preventing the whole driver from crashing.

This is exactly the behavior that you see in vidtv_memcpy and 
vidtv_memset: nothing gets written so we don't end up with an overflow, 
a diagnostic message is printed and there are no attempts at recovery.

In this particular example, I compromised by allowing the size of the 
buffer to be a module param, i.e.

>> +static unsigned int ts_buf_sz = 20 * 188;
>> +module_param(ts_buf_sz, uint, 0644);
>> +MODULE_PARM_DESC(ts_buf_sz, "Optional size for the TS buffer");

I think that what I am trying to say is better seen in the last patch 
for this series: [RFC, WIP, v4 11/11] media: vidtv: Add a MPEG Transport 
Stream Multiplexer.

The following takes place in vidtv_mux.c:

	1. We wake up from sleep, take note of how long we slept for and store 
it into "elapsed_time". This is usually between 10ms and 20ms.
	2. We encode "elapsed_time" miliseconds worth of data into a buffer
	3. We call dvb_dmx_swfilter(), passing a pointer to the buffer
	4. We clear the buffer, sleep for a bit and then go back to 1.

If the buffer is somehow full then it means that we are trying to 
simulate too many fake channels while allocating too little memory, so 
either we scale down on the amount of fake channels or we choose another 
value for the "ts_buf_sz" module_param.

I feel that this is better than adding more logic in an attempt to 
circumvent the issue, specially since I can add more logic and still 
fail due to my limited experience. The result is more bloat on the 
source code for little gain.

> A WARN_ON() seems too severe here. Also, if something bad happens, it
> will end causing lots of problems that can make the machine very slow,
> ad this will flood dmesg.
>
> So, the best would be to use, instead, dev_warn_ratelimited().

You're right, I did not consider this. I will use dev_warn_ratelimited() 
in place of WARN_ON in the next revisions.