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

Reimar Döffinger Reimar.Doeffinger at gmx.de
Tue Sep 17 09:07:06 CEST 2013



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.

> There is no mention of what happens if you do NOT FOLLOW THE SPEC, what has happened in the past, and i have helped fix a couple of dvb subtitle encoders that had this exact problem, is 

Your patch is for the decider, so I do not know how this is relevant.


> I cant see why you 2 are making such a song and dance of "corrupt data ...",

Because that is the _only_ case where your patch is _in any way_ relevant.
If the data follows the specification you quote the current code and the one you change it to _are exactly equivalent_.
if x then a
if y then b

is _only_ different from

if x then a
else if y then b

if both x and y are true.
If the specification says "only one of x and y may be true", how can the specification _require_ (as you claim) that we use the second one?


> Please read the spec again and read the line
> 
> "Only 1 bit SHALL be set"
> 
> It does not say
> 
> "Only 1 bit SHALL be set, but if 2 are this means ............"

Exactly, however by claiming your patch is necessary you actually say the second one, that the invalid case must be treated in a specific way, and we are all asking why....


More information about the ffmpeg-devel mailing list