On 6/30/19, Marton Balint <c...@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. _______________________________________________ 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".