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

[Marton Balint]

On Wed, 26 Jun 2019, Andreas Håkon wrote:

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

I don't understand your reasoning. Sequential mode is not affected if you 
only increase pes_payload_size if parallel mode is used. Is parallel 
mode affected?

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

We generally split features to different patches to make patch review 
easier. Or to put it in a different way, your patch has bigger chance to 
be reviewed and merged if it touches a single feature.

I see no reason why one couldn't use multiple programs in sequential mode 
so it totally makes sense to implement it, and then implement parallel 
mode on top of that as a separete patch.

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

I disagree. Based on your code the 4 modes do this:

< -1  = 0
   -1  = WRITE_HEADER
    0  = WRITE_HEADER | ONE_PACKET_ONLY
>  0  = ONE_PACKET_ONLY

You documented the meaning of the "modes" in two places and created two 
helper functions to make sure you pass the correct modes to the underlying 
function. Why is that if not because you can't seem to remember yourself 
the meaning of modes? Please reconsider using defines or enums.

Thanks,
Marton