[FFmpeg-devel] [PATCH] [soc: dvbmuxer] Improve PCR accuracy
Tomer Barletz
barletz
Mon Dec 29 09:06:10 CET 2008
Hi,
On Thu, Dec 25, 2008 at 7:38 PM, Baptiste Coudurier
<baptiste.coudurier at gmail.com> wrote:
> Tomer Barletz wrote:
>> On Wed, Dec 24, 2008 at 12:44 PM, Baptiste Coudurier
>> <baptiste.coudurier at gmail.com> wrote:
>>> Hi,
>>>
>>> Barletz wrote:
>>>> Hi Baptiste, and thanks for your comments!
>>>>
>>>> On Tue, Dec 23, 2008 at 9:44 PM, Baptiste Coudurier
>>>> <baptiste.coudurier at gmail.com> wrote:
>>>>> Hi,
>>>>>
>>>>> Barletz wrote:
>>>>>> On Tue, Dec 23, 2008 at 1:34 PM, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> Barletz <barletz <at> gmail.com> writes:
>>>>>>>
>>>>>>>> This patch mainly improves PCR accuracy in the current mpegtsenc.c
>>>>>>>> implementation.
>>>>>>> Please use svn diff (or diff -u) to produce the patches.
>>>>>>>
>>>>>>> We can't read other formats;-)
>>>>>>>
>>>>> First, I'd like to thank you for taking the time to work on the muxer,
>>>>> this is really appreciated.
>>>>>
>>>> No problem, the pleasure is mine ;)
>>>>
>>>>> > [...]
>>>>> >
>>>>>> +
>>>>>> + return num_packets_written;
>>>>> Instead of modifying all function to return packets written, would it
>>>>> simpler to add a fiel in the global context ?
>>>>>
>>>>> You can add packet_count to MpegTSWrite, and access it using
>>>>> ->opaque->priv_data in mpegts_write_section.
>>>>>
>>>>> Also please split all modifications that can be splitted. This makes
>>>>> reviewing easier. Thanks.
>>>>>
>>>> I was thinking that this will enhance these methods, since the number
>>>> of packets which were actually flushed to the stream may be important
>>>> in other uses, and per table (perhaps I just want to know how many
>>>> packets the SDT used).
>>> Well, you can check the value before calling the func and after, this is
>>> the same IMHO. I really feel the code would be simpler.
>>>
>>
>> The problem is that I will not know whether any of the mpets_write_XXX
>> has failed. Currently -1 return is used in order to signal for error,
>> but most of the write methods will not delegate this error, since they
>> have no return value.
>
> Changing functions to return -1 on error and propagating this error is
> probably be ok, but this should be in a separate patch.
>
>> I also must disagree on the simpler part - I believe that my way is
>> simpler, I guess It's a matter of taste :D
>
> Only 2 function actually write ts packets: mpegts_write_section1 and
> mpegts_write_pes, so technically only 2 ("ts->packet_count++") are
> needed, then you add the ("ts->cur_pcr = ...") line I pasted and the
> field declaration in the struct and you should be done.
Sorry for misunderstanding. I've attached another patch - please tell
me if this is what you meant. This code does not compile - please tell
me what I'm doing wrong.
>
>>> >>> [...]
>>> >>>
>>>>>> + ts->cur_pcr =
>>>>>> + // the time passed while SI tables written
>>>>>> + MULTI_PACKETS_TIME(num_si_packets_written, ts->mux_rate)
>>>>>> + // the time passed for packets up until the first pcr bit, as
>>>>>> + // required in the standard
>>>>>> + + BYTES_TIME(11, ts->mux_rate);
>>>>>> + write_pcr = 1;
>>>>>> + ++pcr; // done with first time pcr
>>>>> I need to double check this, but why is it simply incremented ?
>>>>>
>>>> I use this variable only to avoid declaring another flag which will
>>>> indicate whether the first pcr is already written - the pcr variable
>>>> will be ignored from now on, and only ts->cur_pcr will be used.
>>> Humm, then can't ts->cur_pcr be checked ? It should be 0 at init.
>>>
>>
>> I've tried that, but the results are different. I figured that some
>> other code is using that value somewhere in between, but I couldn't
>> find it - could you point me to it? Or maybe we could just commit that
>> solution, then later improve it.
>
> I need to double check, this was just an idea, maybe last_pcr could be
> used too.
Any news?
>
>>>>>> + }
>>>>>> + else
>>>>>> + {
>>>>>> + ts->cur_pcr = increment_pcr(ts->cur_pcr,
>>>>>> + // the time passed from last packet
>>>>>> + SINGLE_PACKET_TIME(ts->mux_rate)
>>>>>> + // the time passed while SI tables written
>>>>>> + + MULTI_PACKETS_TIME(num_si_packets_written, ts->mux_rate));
>>>>>> + // write pcr only if pcr pid and if repetition interval is right
>>>>>> + write_pcr = (ts_st->pid == ts_st->service->pcr_pid) ?
>>>>>> + ((ts->cur_pcr - ts->last_pcr) > PCR_REPETITION_INTERVAL ? 1 : 0) : 0;
>>>>> I think header and pcr bytes must be added to pcr value here (11 bytes).
>>>> I added those bytes only when I first encountered a new pcr (pcr==0).
>>>> From then on, I already have this offset, and I just want to increment
>>>> by a single transport packet.
>>>> I've tested the output ts with Tektronics' StreamAnalyzer, and the PCR
>>>> interval and accuracy looks right.
>>> Yes, I see, I think something like this would be more clear to
>>> understand the code:
>>>
>>> ts->cur_pcr = (ts->packet_count*188+11)*8*time_base/ts->mux_rate.
>>>
>>> What do you think ?
>>
>> I think that this is exactly what you see in the current
>> implementation, only in a macro. Do you think that the macro's name is
>> not suitable? Or perhaps I misunderstand you...
>
> No, you are right, This is just what the macro does, only one macro
> could be used though (like PACKET_COUNT_TO_PCR(ts->packet_count)), but
> ideally the +11 should be explained in comment, maybe the macro is not
> needed and the code itself is easier to understand.
I don't mind - as long as we use that macro only once.
>
> --
> Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
> Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
> checking for life_signs in -lkenny... no
> FFmpeg maintainer http://www.ffmpeg.org
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list