[FFmpeg-devel] [PATCH] libavformat/mpegtsenc: new interlaced mux mode

Marton Balint cus at passwd.hu
Sun Jun 23 12:44:57 EEST 2019



On Thu, 13 Jun 2019, Andreas HÃ¥kon wrote:

> Hi Andriy,
>
> I'm glad you're interested in this patch.
>
>
>
>> > This patch implements a new optional "parallel muxing mode" in the MPEGTS muxer.
>> > The strategy that implements the current mux (selected by default) is based on
>> > writing full PES packages sequentially. This mode can be problematic when using
>> > with DTV broadcasts, as some large video PES packets can delay the writing of
>> > other elementary streams.
>>
>> Could you go into more detail as to why this causes problems for DTV broadcasts?
>
> Because two reasons:
>
> 1) When substreams of multiple services are interlaced, broadcast errors are minimized.
> Burst errors in the signal only interfere with a small number (or just one) packet
> of each program, so it's possible that the error is camouflaged.
> In contrast, when using CONSECUTIVE pid packets, burst errors are more significant.
>
> 2) Due to buffer restrictions on DTV broadcasts. If you include more than one video
> stream with large PES packets, sequential writing can cause buffer problems at
> reception because other PES packets arrive a little later.
>

Ok. But I believe the correct term for this is "interleaved". I have 
never heard "interlaced" used for TS packets.

>
>
>> > +#define PES_START 1 /* 0000 0001 /
>> > +#define PES_FULL 2 / 0000 0010 */
>>
>> > +#define UNUSED_FLAG_3 4 /* 0000 0100 /
>> > +#define UNUSED_FLAG_4 8 / 0000 1000 /
>> > +#define UNUSED_FLAG_5 16 / 0001 0000 /
>> > +#define UNUSED_FLAG_6 32 / 0010 0000 /
>> > +#define UNUSED_FLAG_7 64 / 0100 0000 */
>>
>> Is it relevant to include these?
>
> No. It's not necessary. However, they make the code more compressible and can be
> used in the future for other flags.

Don't include them if they are not needed. It would be also a good idea 
to name them xxx_FLAG if these are indeed flags. e.g.:

PES_START_FLAG
PES_FULL_FLAG
PES_NEEDS_END_FLAG

also I see no reason that PES_NEEDS_END is 128. Just make it 4. And keep 
the indentation pretty.

>
>
>> > -          ts_st->payload = av_mallocz(ts->pes_payload_size);
>> > +          ts_st->payload = av_mallocz(ts->parallel_mux ? MAX_PES_PAYLOAD : ts->pes_payload_size);
>>
>> Could you clarify why this needs to be changed?
>
> Sure! Because when you write in parallel you're delaying the writing of the PES packet.
> So you need to save the entire PES packet. And the ts->pes_payload_size it's defined to
> a very small value (2734 Bytes only)... See at the top of the mpegtsenc.c file:
>
> #define DEFAULT_PES_HEADER_FREQ  16
> #define DEFAULT_PES_PAYLOAD_SIZE ((DEFAULT_PES_HEADER_FREQ - 1) * 184 + 170)

OK, but you shold override ts->pes_payload_size, and not the malloc, no? 
There is code in mpegts_init which sets pes_payload_size, you should force 
it to MAX_PES_PAYLOAD there before doing the round up.

>
>
>
>> > +   for (i = 0; i < ts->nb_services; i++) {
>> > +          service = ts->services[i];
>> >   ...
>> >     }
>>
>> Why do you need the loop over the services here? It seems unrelated unless
>> I missed something.
>
> I explained it in my original message...
> The current code has a bug and it doesn't set correctly the value of the
> service->pcr_pid. And this patch requires correct values of pcr_pid for each
> program. Please note that this patch makes complete sense when mixing multiple
> video streams, such as when using multiple programs.

This should be a separate patch then. I suggest implementing this before 
adding interleaving support.

>
>
>
>> > -   -   NOTE: 'payload' contains a complete PES payload.
>> > -   -   NOTE2: When 'mode' < -1 it writes the rest of the payload (without header);
>> > -   -          When 'mode' = -1 it writes the entire payload with the header;
>> > -   -          when 'mode' = 0  it writes only one TS packet with the header;
>> > -   -          when 'mode' > 0  it writes only one TS packet.
>> >
>>
>> Enum for mode would make more sense to me
>
> Yes and not. The code is more compact in this case with numerical values, as you
> don't need to do checks when you call to the function.

The code might be more compact, and tons of less readable. Use enums, or 
defines. You might also use flags for mode, if that makes the code more 
readable.

>
>
>
>> > +          if ((ts_st->pes_flags & PES_NEEDS_END) && payload_size == len) {
>>
>> This seems unrelated to your commit..
>> If it is, you can remove PES_NEEDS_END
>
> Sorry, no! That's completely related to this patch. Let me explain:
>
> Writing Telext PES packets it's an special case. The function writes at end 1
> byte. The flag PES_NEEDS_END is used in this case to indicate this case. Please
> note that the function with this patch is called multiple times for the same PES
> packet. And only when writing the header at the start this check can be determined.
> So we need to save this information. If not, some packets are malformed.
>
>
>
>> > +              payload_size = 0;  // Write one packet only then exit
>>
>> Why not just break?
>
> Sorry? That's the last line of the loop! What's the advantage of breaking the loop here?

Because you break out of it later anyway since the loop condition is 
"payload_size > 0".

Regards,
Marton


More information about the ffmpeg-devel mailing list