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

[Paul B Mahol]

On 6/30/19, Marton Balint <cus at passwd.hu> wrote:
>
>
> 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.
>

Hardcoded values are unacceptable. Every such patch will not be applied
and if someone applies it by accident it will be reverted ASAP.