[FFmpeg-devel] [PATCH] libavformat/mpegtsenc.c -- correctly re-emit extradata ahead of IDR pictures

[Marton Balint]

On Mon, 30 Jan 2023, John Coiner wrote:

> Marton Balint wrote:
>> This does not look right, because AFAIK you always want to insert an AUD
>> unless it is already there. I guess that is why current code works as it
>> is, it assumes if an AUD is already present, then IDR-s are already
>> prefixed with SPS/PPS, and no magic is needed.
>
> I'm not sure if it should be valid to assume that an AUD implies that
> an SPS/PPS will follow,

Well, most likely not, but worked mostly in the past...

> but this is how the above command makes that
> assumption go wrong:
>
>   * The HLS muxer requests global headers as input:
> https://github.com/FFmpeg/FFmpeg/blob/2d202985b79630cd5056c4e32f8f77f22bf1067c/libavformat/hlsenc.c#L3194
>   * This sets the AV_CODEC_FLAG_GLOBAL_HEADER option for the codec:
> https://github.com/FFmpeg/FFmpeg/blob/2d202985b79630cd5056c4e32f8f77f22bf1067c/fftools/ffmpeg_mux_init.c#L599
>   * The libx264.c codec responds to this option by producing
> extradata, and also by configuring x264 not to repeat SPS and PPS
> in-band: https://github.com/FFmpeg/FFmpeg/blob/2d202985b79630cd5056c4e32f8f77f22bf1067c/libavcodec/libx264.c#L1044
>   * When x264 runs with its "aud" option, it emits AUDs.
>   * The AUDs inhibit mpegtsenc.c from emitting the extradata,
> producing the buggy behavior.
>
> An alternative to changing the extradata "magic" in mpegtsenc would be
> for the HLS muxer to re-emit the extradata on its own. The HLS muxer
> has good context for this: it knows when new HLS media segments begin,
> and it knows that each one must begin with SPS and PPS if the encoder
> didn't already include them. It knows it's producing HLS. The
> mpegtsenc muxer doesn't know any of this.
>
> Would it be safer to fix in hlsenc.c and leave mpegtsenc alone? It's
> not clear to me that the mpegtsenc logic is verifiable or falsifiable
> to stronger than a "happens to work" standard; a fix in hlsenc.c could
> be.

For normal mpegts it also makes sense to include SPS/PPS before IDR-s, so 
I'd say it is better if it is fixed in mpegtsenc.

But it is mandatory to insert AUD NAL-s for every frame, and your patch 
breaks that, because it only inserts it if SPS/PPS is also inserted, 
because you changed

if ((state & 0x1f) != 9) { // AUD NAL

to

if (extradd > 0) {

So you need to rework your patch to keep the AUD insertion (but you don't 
want to duplicate it of course).

Regards,
Marton