[FFmpeg-devel] New patch for mpegts.c

Wed Oct 24 20:33:10 CEST 2012

On 24 Oct 2012, at 07:25, JULIAN GARDNER <joolzg at btinternet.com> wrote:
> ----- Original Message -----
>> From: Michael Niedermayer <michaelni at gmx.at>
>> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
>> Cc: 
>> Sent: Wednesday, 24 October 2012, 5:26
>> Subject: Re: [FFmpeg-devel] New patch for mpegts.c
>> 
>> On Wed, Oct 17, 2012 at 05:51:28PM +0100, JULIAN GARDNER wrote:
>>> 
>>> 
>>> 
>>> 
>>> 
>>> ----- Original Message -----
>>>> From: Michael Niedermayer <michaelni at gmx.at>
>>>> To: FFmpeg development discussions and patches 
>> <ffmpeg-devel at ffmpeg.org>
>>>> Cc: 
>>>> Sent: Wednesday, 17 October 2012, 17:36
>>>> Subject: Re: [FFmpeg-devel] New patch for mpegts.c
>>>> 
>>>> On Wed, Oct 17, 2012 at 08:46:07AM +0100, JULIAN GARDNER wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>>   ----- Original Message -----
>>>>>   > From: Michael Niedermayer <michaelni at gmx.at>
>>>>>   > To: FFmpeg development discussions and patches 
>>>> <ffmpeg-devel at ffmpeg.org>
>>>>>   > Cc: 
>>>>>   > Sent: Wednesday, 17 October 2012, 1:41
>>>>>   > Subject: Re: [FFmpeg-devel] New patch for mpegts.c
>>>>>   > 
>>>>>   > Hi
>>>>>   > 
>>>>>   > On Tue, Oct 16, 2012 at 08:34:27PM +0100, JULIAN GARDNER 
>> wrote:
>>>>>   >> 
>>>>>   >>  >________________________________
>>>>>   >>  > From: Michael Niedermayer <michaelni at gmx.at>
>>>>>   >>  >To: FFmpeg development discussions and patches 
>>>>>   > <ffmpeg-devel at ffmpeg.org> 
>>>>>   >>  >Sent: Tuesday, 16 October 2012, 17:16
>>>>>   >>  >Subject: Re: [FFmpeg-devel] New patch for mpegts.c
>>>>>   >>  > 
>>>>>   >>  >On Tue, Oct 16, 2012 at 02:33:01PM +0100, JULIAN 
>> GARDNER 
>>>> wrote:
>> [...]
>>>>>   > If its about spec compliance, please quote the spec that 
>> requires
>>>>>   > this behavior.
>>>>>   > 
>>>>>   ETS 300468  5.1.1 d  
>>>> 
>> http://www.etsi.org/deliver/etsi_en/300400_300499/300468/01.11.01_60/en_300468v011101p.pdf
>>>> 
>>>> doesnt say one should discard things with equal version numbers
>>>> 
>>> 
>>> "When the characteristics of the TS described in the SI given in the 
>> present document change (e.g. new events start, different composition of 
>> elementary streams for a given service), then new SI data shall be sent 
>> containing the updated information. A new version of the SI data is signalled by 
>> sending a sub_table with the same identifiers as the previous sub_table 
>> containing the relevant data, but with the next value of version_number."
>>> 
>>> "document change" " the next value of version_number", 
>> so to me this means that if the data is THE SAME then the version number will be 
>> the same, and i guess that the guys who wrote the spec did this so you DONT have 
>> to process the SAME data over and over again, what it was added for was hardware 
>> filtering, which if you have spent 15+ years writing code for sat receivers etc 
>> you will know this. Why would you want to process possibly millions of sections 
>> which contain the SAME data, to just produce the same tables?.
>>> 
>>> Now if you keep on banging on about the .00000001% of people who 
>> concatenate 2 ts streams and do not follow the spec and either make sure the SI 
>> data is the same, which means the version number the same, or make the version 
>> number follow by becoming the next version then we are at an impasse, because i 
>> think your wrong on this, but as your in charge i will await your decision.
>>> 
>>> And what are we discard, we are just not processing THE SAME DATA over and 
>> over again.
>> [...]
>>>>>   Again we either process xxxx thousand sections or we do as the 
>> spec says 
>>>> and process only new/updated?
>>>> 
>>>> The spec doesnt say that, it says:
>>>>     d) version_number:
>>>>    -     When the characteristics of the TS described in the SI given 
>> in the 
>>>> present document change (e.g. new
>>>>          events start, different composition of elementary streams for 
>> a given 
>>>> service), then new SI data shall be
>>>>          sent containing the updated information. A new version of the 
>> SI data 
>>>> is signalled by sending a sub_table
>>>>          with the same identifiers as the previous sub_table 
>> containing the 
>>>> relevant data, but with the next value
>>>>          of version_number.
>>>>    -     For the SI tables specified in the present document, the 
>> version_number 
>>>> applies to all sections of a
>>>>          sub_table.
>>>> 
>>>> That speaks about "when X then ... shall be sent ...", 
>> sending happens
>>>> on the muxer side, not the demuxer. The text quoted says nothing about
>>>> what a demuxer should or should not do with what it receives. It
>>>> simply describes what a demuxer can expect from a valid DVB stream.
>>>> A concatenated stream as you already explained is not a valid DVB
>>>> stream ...
>>> 
>>> So your now saying that the spec is only for the muxer, can you show me the 
>> spec for the demuxer then that is different, again i come back to the "New 
>> version .... but with the next value of version number". This spec if for 
>> DVB compatible systems, both muxers and demuxers.
>>> 
>>> So have you tested what happens when you play a concatenated stream on a 
>> DVB compliant receiver, you will get the same as with my modifications. As you 
>> always seem to tell people, fix the problem 1st. The only problem i can see is 
>> that you want the decoder NOT to follow the spec, but instead work for the NON 
>> COMPATIBLE streams, when instead the concatenate should be fixed to produce 
>> correct TS streams.
>>> 
>>> I will leave it now and let you decide, as this circle will just keep going 
>> round and round. If it will be included in any future ffmpeg release then great 
>> if not then i will just keep it in my local tree.
>> 
>> Patches must provide an overall improvment to be accepted.
>> Noone has shown a meassureable improvment for it yet and
>> the patch breaks seeking and breaks concatenated streams
>> If you want to keep it in its current form in your local tree then
>> i think you make a mistake.
>> 
>> Instead IMHO you should rethink what improvment the patch provides
>> and then objectively test if it really does. If it does, then fix
>> the issues and repost the patch so it gets included in ffmpeg and
>> all users benefit.
>> OTOH if it doesnt provide a real user vissible improvment, lets just
>> lay it rest and work on something that does.
>> 
> 
> Ok given up trying to convince you that you are wrong.

For what it's worth I can't see where it would be wrong.
By what I understand of it I can see only one way this can be used to definitely improve the code: actually check if the data that should be the same actually is, and if not use a heuristic to decide which to use.
This would not change anything for concatenated streams if done correctly, while being more resilient to bitstream errors.
Concerning the interpretation of the spec: I see it only making assurances, which means that e.g. as an optimization a demuxer can skip some processing with risking issues. That does however not say anything about what a demuxer should do when those assurances are actually violated.