[FFmpeg-devel] Small modifcation to libavformat/dvbsubdec.c

[Reimar Döffinger]

JULIAN GARDNER <joolzg at btinternet.com> wrote:
>
>
>
>
>----- Original Message -----
>> From: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
>> To: FFmpeg development discussions and patches
><ffmpeg-devel at ffmpeg.org>
>> Cc: 
>> Sent: Tuesday, 17 September 2013, 9:32
>> Subject: Re: [FFmpeg-devel] Small modifcation to
>libavformat/dvbsubdec.c
>> 
>>& quot;Reimar Döffinger" <Reimar.Doeffinger at gmx.de> wrote:
>>> 
>>> 
>>> On 17.09.2013, at 08:06, JULIAN GARDNER <joolzg at btinternet.com>
>wrote:
>>>>  ----- Original Message -----
>>>>>  From: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
>>>>>  To: FFmpeg development discussions and patches
>>> <ffmpeg-devel at ffmpeg.org>
>>>>>  Cc: "ffmpeg-devel at ffmpeg.org" 
>> <ffmpeg-devel at ffmpeg.org>
>>>>>  Sent: Tuesday, 17 September 2013, 7:26
>>>>>  Subject: Re: [FFmpeg-devel] Small modifcation to
>>> libavformat/dvbsubdec.c
>>>>> 
>>>>> 
>>>>> 
>>>>>  On 16.09.2013, at 23:58, Carl Eugen Hoyos <cehoyos at ag.or.at> 
>> wrote:
>>>>> 
>>>>>>  JULIAN GARDNER <joolzg <at> btinternet.com> writes:
>>>>>> 
>>>>>>>  Currently in my own personal tree are
>>>>>>> 
>>>>>>>  Complete
>>>>>>>  1. DVB Teletext language parsing
>>>>>>>  2. DVB Teletext SI generation for -c:s copy
>>>>>> 
>>>>>>  Please send patches, or even better, setup a git 
>>>>>>  clone and ask Michael to merge. I know that these 
>>>>>>  features are very welcome!
>>>>>> 
>>>>>>  I may (completely) misunderstand this thread but 
>>>>>>  I suspect patches that make decoders more strict 
>>>>>>  than absolutely required are generally not a 
>>>>>>  good idea, particularly if no sample is known 
>>>>>>  that profits from the change.
>>>>> 
>>>>>  The patch doesn't make it more strict.
>>>>>  A patch making it more strict would do something like adding
>>>>>  if (!!(depth & 0x80) + !!(depth & 0x40) + !!(depth & 
>> 0x20) > 1)
>>>>>    return AVERROR_INVALID_DATA;
>>>>> 
>>>>>  What it does instead is that it interprets an invalid value like
>>> 0xc0 as 0x80.
>>>>>  The spec (at least the quoted part) does not say a thing about
>how
>>> corrupt data 
>>>>>  should be handled as far as I can tell, and 3 of us (at least 2 
>> with
>>> extensive 
>>>>>  experience reading and implementing specifications) agree on
>that,
>>> so while 
>>>>>  experience is good and something to respect the reasoning for the
>>> change makes 0 
>>>>>  sense to us so far.
>>>>>  The commit message really should explain why a value of 0xc0
>should
>>> be treated 
>>>>>  like 0x80 and not e.g. 0x40 or 0x00, just as examples.
>>>> 
>>>>  Spec: Only 1 bit SHALL be set. IS THIS NOT CLEAR? You are making
>>> something out of the spec that does not exist, dont make up stuff,
>use
>>> the spec as it is written.
>>> 
>>> I suggest you try to read what I wrote with a calm mind, you are
>being
>>> _extremely_ insulting.
>> 
>> And just in case we really are idiots: maybe you can just give a
>specific 
>> example of a value of the depth variable where your patch fixes
>behaviour?
>> I am fairly certain we all understand what that sentence says, we
>just see it as 
>> confirming that your patch should not make a difference.
>
>Now insinuating that i think you are IDIOTS is not nice,

That wasn't my intention. I was just seriously considering the possibility of me being an idiot there...

> My problem is
>that you all read too much in a spec, the spec says X you say X?? what
>about Y, when the spec does not mention Y.

My understanding is that your patch is about handling Y, so of course we are asking about Y and why you want it to be handled just this way and why it is better than what we do currently.
I think I see the point why from the spec only setting one might seem more logical, but it doesn't explain why setting just the first one instead of only the last one or even none.
But from your other mail I concluded the answer is "because it works better with real-world samples".
If so that was all I think we wanted to know. We, and I personally wanted to know if we can/should raise an error when strict compliance checking is requested.

>If I thought you were IDIOTS i would have moved over to libav, which if
>you want I will gladly do as it seems my inputs cause too much fraction
>in your group.

I don't think there is any fraction, I just feel like I completely failed to explain to you what we wanted to know and why your arguments did not make sense to us...
I hope I am not on the wrong track again with my current theory...