Re: [FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets
On Tue, 4 Jun 2019, Andreas Håkon wrote: Hi Marton, Thank you for taking the time to review this patch! ‐‐‐ Original Message ‐‐‐ On Sunday, 2 de June de 2019 1:38, Marton Balint wrote: On Wed, 22 May 2019, Andreas Håkon wrote: > Hi, > Patch to generate aligned Teletext PES packets using the MPEG-TS muxer > when the TS header contains data. > The code that generates the PES packets for Teletext data aligns the PES packets > with the boundaries of the TS packets. The strategy used is to insert padding > stuff into the PES header. The size of this padding alignment currently has a > fixed size of 36 bytes. And this value is correct when all of the Teletext > TS packets have payload only. However, inserting some data into the TS header > breaks this assumption. In this case, the Teletext PES packets are not aligned > and the end of the PES packet is inserted into an additional TS packet with a > large amount of TS padding. What is the data that is inserted into the TS header? Can you give an example? Are you sure that it is allowed to generate such streams? Are you seeing such streams in the wild? It can vary. For example you can add PCR timestamps inside the Teletext stream (not very common, but possible). The teletext specs also says, that a DVB teletext stream adaptation_field_control is either 01 or 10, which means that either there is an adaptation field or there is payload. Having both is NOT allowed. If PCR needs to be transmitted, then it either has to be on a separate PCR PID or if you insist on using the same PID then you must use a separate packet with adaptation field only if you want to respect the specification. The DVB teletext spec is very strict about that PES header size and 0x24 is hardcoded there. https://www.etsi.org/deliver/etsi_en/300400_300499/300472/01.03.01_60/en_300472v010301p.pdf Yes, you're right. The sentence at section "4.2 PES packet format" indicates: "PES_header_data_length set to "0x24"." And after it adds: "PTS and other optional fields may be present in the PES header, but the header length is always fixed for streams identified in the Program Specific Information (PSI) by the DVB Teletext descriptor" However, the idea of this restriction is to leave space for PTS data even if it's not present in the PES packet. And if you don't add any additional stuff in the TS header, the constraint is satisfied. Perhaps you'd prefer this behavior to be optional? Our mpegts muxer should not generate streams which are not compliant with the relevant specifications. So the proper way to do what you want seems to be generating adaptation field only packets. Regards, Marton ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets
Hi Marton, Thank you for taking the time to review this patch! ‐‐‐ Original Message ‐‐‐ On Sunday, 2 de June de 2019 1:38, Marton Balint wrote: > On Wed, 22 May 2019, Andreas Håkon wrote: > > > Hi, > > Patch to generate aligned Teletext PES packets using the MPEG-TS muxer > > when the TS header contains data. > > > The code that generates the PES packets for Teletext data aligns the PES > > packets > > with the boundaries of the TS packets. The strategy used is to insert > > padding > > stuff into the PES header. The size of this padding alignment currently has > > a > > fixed size of 36 bytes. And this value is correct when all of the Teletext > > TS packets have payload only. However, inserting some data into the TS > > header > > breaks this assumption. In this case, the Teletext PES packets are not > > aligned > > and the end of the PES packet is inserted into an additional TS packet with > > a > > large amount of TS padding. > > What is the data that is inserted into the TS header? Can you give an > example? Are you sure that it is allowed to generate such streams? > Are you seeing such streams in the wild? It can vary. For example you can add PCR timestamps inside the Teletext stream (not very common, but possible). > The DVB teletext spec is very strict about that PES header size and 0x24 > is hardcoded there. > > https://www.etsi.org/deliver/etsi_en/300400_300499/300472/01.03.01_60/en_300472v010301p.pdf Yes, you're right. The sentence at section "4.2 PES packet format" indicates: "PES_header_data_length set to "0x24"." And after it adds: "PTS and other optional fields may be present in the PES header, but the header length is always fixed for streams identified in the Program Specific Information (PSI) by the DVB Teletext descriptor" However, the idea of this restriction is to leave space for PTS data even if it's not present in the PES packet. And if you don't add any additional stuff in the TS header, the constraint is satisfied. Perhaps you'd prefer this behavior to be optional? > So maybe you should use adaptation field stuffing instead to keep the > alignment? This is already being done. The objective is to use the empty space of the PES header when there is data in the TS header, maintaining the original alignment. > > + int header_stuff = (get_ts_payload_start(buf) - buf) - 4; > > This is a very bad variable name. Call it adaptation_field_len. OK. > > + if (header_stuff - header_len > 0x24) > > + header_stuff = 0; > > Are you sure about this check? YES. When no free space in the PES header then the code is disabled (equivalent to "adaptation_field_len"/"header_stuff" = 0)... see this line: > > + header_len = 0x24 - header_stuff; And the aligment of the last TS packet is done using TS header stuffing. So, please Marton, comment if you prefer: - Change only the ugly name of the var. - Make behaviour optional. - Discard the patch. Regards. A.H. --- ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets
On Wed, 22 May 2019, Andreas Håkon wrote:
Hi,
Patch to generate aligned Teletext PES packets using the MPEG-TS muxer
when the TS header contains data.
The code that generates the PES packets for Teletext data aligns the PES packets
with the boundaries of the TS packets. The strategy used is to insert padding
stuff into the PES header. The size of this padding alignment currently has a
fixed size of 36 bytes. And this value is correct when all of the Teletext
TS packets have payload only. However, inserting some data into the TS header
breaks this assumption. In this case, the Teletext PES packets are not aligned
and the end of the PES packet is inserted into an additional TS packet with a
large amount of TS padding.
What is the data that is inserted into the TS header? Can you give an
example? Are you sure that it is allowed to generate such streams?
Are you seeing such streams in the wild?
The DVB teletext spec is very strict about that PES header size and 0x24
is hardcoded there.
https://www.etsi.org/deliver/etsi_en/300400_300499/300472/01.03.01_60/en_300472v010301p.pdf
So maybe you should use adaptation field stuffing instead to keep the
alignment?
This patch calculates the size of the TS header in order to reduce the size of
the PES padding and thus accommodate the PES packet to the TS boundaries.
In this way no additional TS packets are produced.
Signed-off-by: Andreas Hakon
libavformat/mpegtsenc.c |7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index fc0ea22..f1772ac 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -1328,8 +1328,11 @@ static void mpegts_write_pes(AVFormatContext *s,
AVStream *st,
header_len += 3;
}
if (is_dvb_teletext) {
-pes_header_stuffing_bytes = 0x24 - header_len;
-header_len = 0x24;
+int header_stuff = (get_ts_payload_start(buf) - buf) - 4;
This is a very bad variable name. Call it adaptation_field_len.
+if (header_stuff - header_len > 0x24)
+header_stuff = 0;
Are you sure about this check?
+pes_header_stuffing_bytes = 0x24 - header_len - header_stuff;
+header_len = 0x24 - header_stuff;
}
len = payload_size + header_len + 3;
/* 3 extra bytes should be added to DVB subtitle payload: 0x20
0x00 at the beginning and trailing 0xff */
--
1.7.10.4
Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets
On Tue, May 28, 2019 at 05:48:27PM +, Andreas Håkon wrote: > Hi Michael, > > > with which software has the generated mpeg-ts been tested ? > > do you have any testecases ? > > Our test cases are mainly DVB-C broadcasts and mobile streaming. > All clients play well. And internal testing is done using the > well-known DVB Inspector. adding information about testing or other things that increase the confidence of a reviewer that a change is correct should decrease the time until its applied thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Elect your leaders based on what they did after the last election, not based on what they say before an election. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets
Hi Michael, > with which software has the generated mpeg-ts been tested ? > do you have any testecases ? Our test cases are mainly DVB-C broadcasts and mobile streaming. All clients play well. And internal testing is done using the well-known DVB Inspector. Regards. A.H. --- ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets
On Tue, May 28, 2019 at 12:08:55PM +, Andreas Håkon wrote: > Hi A. Rheinhardt, > > > ‐‐‐ Original Message ‐‐‐ > On Tuesday, 28 de May de 2019 13:24, Andreas Rheinhardt > wrote: > > > > Can someone please tell me who is the official maintainer of the MPEG-TS > > > muxer? > > > I'm still waiting for comments regarding this patch. > > > We've been using it for a long time without problems. > > > > Just take a look into the MAINTAINERS file: > > > > mpegts.c Marton Balint > > mpegtsenc.c Baptiste Coudurier > > I do that! More than two weeks ago I send a private message to B. Coudurier > (free.fr) mail. > However, I think that address doesn't work. > > I hope someone can review and merge this patch. > Sorry for disturbing you. with which software has the generated mpeg-ts been tested ? do you have any testecases ? thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Complexity theory is the science of finding the exact solution to an approximation. Benchmarking OTOH is finding an approximation of the exact signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets
Andreas Håkon: > Hi A. Rheinhardt, > > > ‐‐‐ Original Message ‐‐‐ > On Tuesday, 28 de May de 2019 13:24, Andreas Rheinhardt > wrote: > >>> Can someone please tell me who is the official maintainer of the MPEG-TS >>> muxer? >>> I'm still waiting for comments regarding this patch. >>> We've been using it for a long time without problems. >> >> Just take a look into the MAINTAINERS file: >> >> mpegts.c Marton Balint >> mpegtsenc.c Baptiste Coudurier > > I do that! More than two weeks ago I send a private message to B. Coudurier > (free.fr) mail. > However, I think that address doesn't work. His emails to this list are sent from baptiste.coudur...@gmail.com. - Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets
Hi A. Rheinhardt, ‐‐‐ Original Message ‐‐‐ On Tuesday, 28 de May de 2019 13:24, Andreas Rheinhardt wrote: > > Can someone please tell me who is the official maintainer of the MPEG-TS > > muxer? > > I'm still waiting for comments regarding this patch. > > We've been using it for a long time without problems. > > Just take a look into the MAINTAINERS file: > > mpegts.c Marton Balint > mpegtsenc.c Baptiste Coudurier I do that! More than two weeks ago I send a private message to B. Coudurier (free.fr) mail. However, I think that address doesn't work. I hope someone can review and merge this patch. Sorry for disturbing you. Regards. A.H. --- ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets
Andreas Håkon: > Hi, > > ‐‐‐ Original Message ‐‐‐ > On Wednesday, 22 de May de 2019 17:04, Andreas Håkon > wrote: > >> Patch to generate aligned Teletext PES packets using the MPEG-TS muxer >> when the TS header contains data. > > Can someone please tell me who is the official maintainer of the MPEG-TS > muxer? > I'm still waiting for comments regarding this patch. > We've been using it for a long time without problems. Just take a look into the MAINTAINERS file: mpegts.c Marton Balint mpegtsenc.c Baptiste Coudurier - Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets
Hi, ‐‐‐ Original Message ‐‐‐ On Wednesday, 22 de May de 2019 17:04, Andreas Håkon wrote: > Patch to generate aligned Teletext PES packets using the MPEG-TS muxer > when the TS header contains data. Can someone please tell me who is the official maintainer of the MPEG-TS muxer? I'm still waiting for comments regarding this patch. We've been using it for a long time without problems. Regards. A.H. --- ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets
Hi,
Patch to generate aligned Teletext PES packets using the MPEG-TS muxer
when the TS header contains data.
Regards.
A.H.
---From 5b476153e0fcced53afa1b77543c2e386f08c504 Mon Sep 17 00:00:00 2001
From: Andreas Hakon
Date: Wed, 22 May 2019 15:36:50 +0100
Subject: [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES
packets
The code that generates the PES packets for Teletext data aligns the PES packets
with the boundaries of the TS packets. The strategy used is to insert padding
stuff into the PES header. The size of this padding alignment currently has a
fixed size of 36 bytes. And this value is correct when all of the Teletext
TS packets have payload only. However, inserting some data into the TS header
breaks this assumption. In this case, the Teletext PES packets are not aligned
and the end of the PES packet is inserted into an additional TS packet with a
large amount of TS padding.
This patch calculates the size of the TS header in order to reduce the size of
the PES padding and thus accommodate the PES packet to the TS boundaries.
In this way no additional TS packets are produced.
Signed-off-by: Andreas Hakon
libavformat/mpegtsenc.c |7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index fc0ea22..f1772ac 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -1328,8 +1328,11 @@ static void mpegts_write_pes(AVFormatContext *s,
AVStream *st,
header_len += 3;
}
if (is_dvb_teletext) {
-pes_header_stuffing_bytes = 0x24 - header_len;
-header_len = 0x24;
+int header_stuff = (get_ts_payload_start(buf) - buf) - 4;
+if (header_stuff - header_len > 0x24)
+header_stuff = 0;
+pes_header_stuffing_bytes = 0x24 - header_len - header_stuff;
+header_len = 0x24 - header_stuff;
}
len = payload_size + header_len + 3;
/* 3 extra bytes should be added to DVB subtitle payload: 0x20
0x00 at the beginning and trailing 0xff */
--
1.7.10.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
