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

[Andreas Håkon]

Hi Marton,

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

Don't worry!

First of all three points:
1. For the Sequential mode we CAN'T change the value of pes_payload_size.
2. For the Sequential mode we CAN increase the size of the malloc for the ts_st->payload.
   But this is useless (a wasted allocated space).
3. If we increase the size of ts->pes_payload_size this will affect to both modes.

Why? Perhaps your problem is this: the value of ts->pes_payload_size in the current code
doesn't represents the MAX payload size of a PES message, but the MINIMUM size for a
PES message. However, for the new Parallel mode, we need to save the total PES message
until the last part of it is writed. And for this objective we save it in the
ts_ts->payload; and the size in this case requires to be the MAX_PES_PAYLOAD.

Futhermore, if we increase the size of ts->pes_payload_size this will affect to
both modes.

Just in one line: "ts->pes_payload_size" IS NOT the size of the PES packet !!!

More clear now?



> > > > > > -   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 understand it. However, my personal objective is to make this patch fit the criteria
for acceptance. I published here other patches, but several of them are not accepted.
I don't want to discuss your criteria, and I prefer to accept it. However, I don't have
the time to fix something that I don't need it. So, I pointed it here as the bug exists
and this patch fixes it.

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

Sure, but if this patch is accepted the bug is resolved too.



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

Sorry! I think you're mixing several different topics:

The code pointed here DON'T HAVE ANY DIRECT RELATION with the previous comment
in the header of the function "mpegts_write_pes()". This code is internal in the
function "write_side_streams()". This last function uses an internal variable
write_mode that is used in the case switch to make the code less complex. Note
that the function receives the parameter "parallel". So the value of the
"write_mode" is calculated in multiple parts of the internal code. If I remove
this simple "internal" comments I'm sure you don't see any relation between them.

I prefer to stop commenting about the NUMERICAL value of the parameter "mode" in
the function "mpegts_write_pes()". If it's really required I'll change it to a
DEFINE value.

In any case, please continue with the new version of the patch posted days ago:
https://patchwork.ffmpeg.org/patch/13745/

Regards.
A.H.

---