From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> To: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Cc: sean@mess.org, kstewart@linuxfoundation.org, allison@lohutok.net, tglx@linutronix.de, linux-media@vger.kernel.org, skhan@linuxfoundation.org, linux-kernel-mentees@lists.linuxfoundation.org, linux-kernel@vger.kernel.org Subject: Re: [RFC, WIP, v4 07/11] media: vidtv: add MPEG TS common code Date: Sat, 2 May 2020 19:22:06 -0300 [thread overview] Message-ID: <de298533-e002-99f0-0db0-0418a284fa9e@gmail.com> (raw) In-Reply-To: <20200502090933.171aa862@coco.lan> Hi Mauro, thanks for reviewing this! > Em Sat, 2 May 2020 00:22:12 -0300 > "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu: > >> From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> >> >> 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@gmail.com> >> --- >> 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@gmail.com> >> + */ >> + >> +#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.
WARNING: multiple messages have this Message-ID (diff)
From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> To: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Cc: kstewart@linuxfoundation.org, sean@mess.org, linux-kernel@vger.kernel.org, tglx@linutronix.de, linux-kernel-mentees@lists.linuxfoundation.org, allison@lohutok.net, linux-media@vger.kernel.org Subject: Re: [Linux-kernel-mentees] [RFC, WIP, v4 07/11] media: vidtv: add MPEG TS common code Date: Sat, 2 May 2020 19:22:06 -0300 [thread overview] Message-ID: <de298533-e002-99f0-0db0-0418a284fa9e@gmail.com> (raw) In-Reply-To: <20200502090933.171aa862@coco.lan> Hi Mauro, thanks for reviewing this! > Em Sat, 2 May 2020 00:22:12 -0300 > "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu: > >> From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> >> >> 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@gmail.com> >> --- >> 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@gmail.com> >> + */ >> + >> +#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. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2020-05-02 22:22 UTC|newest] Thread overview: 69+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-02 3:22 [RFC, WIP, v4 00/11] media: vidtv: implement a virtual DVB driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 3:22 ` [RFC, WIP, v4 01/11] media: vidtv: add Kconfig entry Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 4:58 ` Mauro Carvalho Chehab 2020-05-02 4:58 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 02/11] media: vidtv: implement a tuner driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 5:27 ` Mauro Carvalho Chehab 2020-05-02 5:27 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 03/11] media: vidtv: implement a demodulator driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 5:58 ` Mauro Carvalho Chehab 2020-05-02 5:58 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 04/11] media: vidtv: move config structs into a separate header Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 6:02 ` Mauro Carvalho Chehab 2020-05-02 6:02 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 9:28 ` kbuild test robot 2020-05-02 3:22 ` [RFC, WIP, v4 05/11] media: vidtv: add a bridge driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 6:30 ` Mauro Carvalho Chehab 2020-05-02 6:30 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 21:12 ` Daniel W. S. Almeida 2020-05-02 21:12 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 10:05 ` kbuild test robot 2020-05-02 3:22 ` [RFC, WIP, v4 06/11] media: vidtv: add wrappers for memcpy and memset Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 6:40 ` Mauro Carvalho Chehab 2020-05-02 6:40 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-03 7:06 ` Mauro Carvalho Chehab 2020-05-03 7:06 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 07/11] media: vidtv: add MPEG TS common code Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 7:09 ` Mauro Carvalho Chehab 2020-05-02 7:09 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 22:22 ` Daniel W. S. Almeida [this message] 2020-05-02 22:22 ` Daniel W. S. Almeida 2020-05-03 9:50 ` Mauro Carvalho Chehab 2020-05-03 9:50 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 08/11] media: vidtv: implement a PSI generator Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-03 7:51 ` Mauro Carvalho Chehab 2020-05-03 7:51 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-06 6:28 ` Daniel W. S. Almeida 2020-05-06 6:28 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-06 8:36 ` Mauro Carvalho Chehab 2020-05-06 8:36 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 09/11] media: vidtv: implement a PES packetizer Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-03 8:16 ` Mauro Carvalho Chehab 2020-05-03 8:16 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-06 6:55 ` Daniel W. S. Almeida 2020-05-06 6:55 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-06 8:59 ` Mauro Carvalho Chehab 2020-05-06 8:59 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 10/11] media: vidtv: Implement a SMPTE 302M encoder Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-03 8:57 ` Mauro Carvalho Chehab 2020-05-03 8:57 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 11/11] media: vidtv: Add a MPEG Transport Stream Multiplexer Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 9:41 ` kbuild test robot 2020-05-03 9:13 ` Mauro Carvalho Chehab 2020-05-03 9:13 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-06 7:05 ` Daniel W. S. Almeida 2020-05-06 7:05 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-06 9:01 ` Mauro Carvalho Chehab 2020-05-06 9:01 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab
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=de298533-e002-99f0-0db0-0418a284fa9e@gmail.com \ --to=dwlsalmeida@gmail.com \ --cc=allison@lohutok.net \ --cc=kstewart@linuxfoundation.org \ --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=sean@mess.org \ --cc=skhan@linuxfoundation.org \ --cc=tglx@linutronix.de \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.