[FFmpeg-devel] [PATCH] [soc: dvbmuxer] Improve PCR accuracy

Tomer Barletz barletz
Mon Dec 29 09:06:51 CET 2008


On Mon, Dec 29, 2008 at 10:06 AM, Tomer Barletz <barletz at gmail.com> wrote:
> 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
>>
>

This time with the file...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mpegtsenc.diff
Type: text/x-patch
Size: 5043 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081229/18ea8f82/attachment.bin>



More information about the ffmpeg-devel mailing list