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

[Andreas Håkon]

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.



> > +#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.



> > -          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)



> > +   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.



> > -   -   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.



> > +          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?
I would understand the meaning of a return, but we need to execute the last part of the
code before the end return. So a return is out of scope.
We'd better make the code clear and simple.



> >     --
>
> Andriy

More questions?
A.H.

---