[FFmpeg-devel] [PATCHv2 5/6] avformat/mpegtsenc: fix PCR generation intervals

[Marton Balint]

On Sun, 11 Aug 2019, Andreas Håkon wrote:

> Hi Marton,
>
>
>> > This new version LGTM.
>
>> Thanks. I did a slight change for VBR streams and tried to mimic the old
>> behaviour when selecting a PCR interval. Will post an updated version, but
>> that should only affect VBR.
>
> Regarding this new VBR patch I have one comment...
>
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Saturday, 10 de August de 2019 22:27, Marton Balint <cus at passwd.hu> wrote:
>
>> PCR generation was based on counting packets for both CBR and VBR streams.
>> Couting packets might have worked for CBR streams (when muxrate was specified)
>> but it only took into account the packets of a service (or the packets of the
>> PCR stream lately), so even that was problematic for multi program streams.
>>
>> The new code works on actual timestamps for both CBR and VBR streams. For VBR
>> streams the behaviour of the old code is simulated by selecting a PCR interval
>> which is the highest multiple of the frame duration but still less than 100 ms.
>>
>> It should be trivial to add support for setting the PCR interval for VBR
>> streams as well in a later patch.
>>
>> The accuracy of PCR packets for CBR streams was greatly improved by preemtively
>> sending them at PCR intervals even if sending the payload of another stream
>> is in progress.
>>
>> Signed-off-by: Marton Balint cus at passwd.hu
>>
>> libavformat/mpegtsenc.c | 90 +++++++++++++++++++++++++++++--------------------
>> 1 file changed, 54 insertions(+), 36 deletions(-)
>>
>> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
>
>> [...]
>
>> +          /* For VBR we select the highest multiple of frame duration which is less than 100 ms. */
>
> Don't you think it's a good idea to use the parameter "pcr_period" as the limit,
> instead of a fixed value of 100ms?

Yes, but I think it is better make that feature a separate patch.

>
>> [...]
>
>
> And I think you've forgotten the file "muxers.texi" in this v2 version of the patch.
> (the original patch 5 has it).

That is intentional. The existing behaviour is preserved, so no change is 
required in the docs. When I make a separate patch to add support for 
setting pcr_period for VBR (should be trivial to do on top of the existing 
patchset), then I will update the docs.

Regards,
Marton