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

[Andreas Håkon]

Hi Marton,


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

I feel you're right: the name "interleaved mux mode" is preferable.
Thank you for pointing it out!


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

OK. I agree.

> also I see no reason that PES_NEEDS_END is 128. Just make it 4.

Ummm... The reason for leaving values unused is for future improvements.
If another developer needs to use a new FLAG, it is mandatory to avoid collisions.

Perhaps a simple comment like that is preferable:
/* Unused free flags: 4,8,16,32,64 */

> And keep the indentation pretty.

In the patch the identation is correct!


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

No, no! If I override the ts->pes_payload_size then when not using the
interleaved some unused memory will be allocated. Please note that when using the
sequential mode the payload is writed "directly" to the output stream. And this is
true with PES packets with a size greater than pes_payload_size (i.e. video). So
the ts->pes_payload_size is used only to store small writes until a PES packet
is complete. However, when using the interleaved mode the entire PES packet requires
to be saved until the last part is writed in the output stream. For this reason the
malloc has different values.


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

Yes and not...

1. This interleaved mode requires correct value of the service->pcr_pid, as it have
sense with multiple programs. So the "patch" here is mandatory. We can assume that
it's part of the new mux mode.

2. With the sequential mode, the service->pcr_pid bug is hidden, because no one uses
multiple programs. So a lot of work for me for an irrelevant fix.

So I'd prefer to just comment here, and make it all one single change.


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


I'm sorry, no. In my opinion it's more legible and compact like that.
I accept all your comments, and I appreciate them. But in this case I do not
see the advantage.


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

OK. With a "break;" instead of "payload_size = 0;" it will be the same.


Thank you for reviewing!
Regards.
A.H.

---