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

JULIAN GARDNER joolzg at btinternet.com
Tue Sep 17 09:42:09 CEST 2013





----- 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:07
> Subject: Re: [FFmpeg-devel] Small modifcation to libavformat/dvbsubdec.c
> 
> 
> 
> 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.

insulting, how because i asked you to read the spec.

>>  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.

Encoders and Decoders work TOGETHER, if one is wrong the the other invariable is wrong, unless you want both to not adhere to specs. think in simple terms

decode the data INCORRECTLY. as will happen with the old code
encode the INCORRECT data ...... oh and then your STB will not display the data right

or

decode the data correctly
encode the CORRECT data

> 
> 
>>  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 ............"
> 

Twist the words around all you like, this stops the case of an buggy encoder setting 2 values which are needed in the 8->4->2 bit conversion routines.

Now I dont know how many boxes are deployed with your DVB Subtitles engine, but my code is running in 100000+ boxes and when did i have to fix my code in the last 10 years, 0 times, how many broadcast dvb encoders have i helped fix in 10 years, TWO, and guess what both were doing, setting multiple cluts at once, and what happened to cause the failure, boxes that could not display 8bit colour and did the 8->4 conversion failed, and also boxes that had to do the 8bit->2bit conversion failed

So YES this is a valid modification, but as was pointed out experience is no place against reading a spec?

, > 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....

Anyway for such a trivial patch to have taken so much time up, ill forgo pushing any more patches.

joolz


More information about the ffmpeg-devel mailing list