[FFmpeg-devel] [PATCH] libavformat/mpegtsenc: adaptive alignment for teletext PES packets

[Andreas Håkon]

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 <cus at passwd.hu> 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.

---