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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
