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

[Reimar Döffinger]

On 16.09.2013, at 21:36, Michael Niedermayer <michaelni at gmx.at> wrote:

> On Mon, Sep 16, 2013 at 08:16:53PM +0100, JULIAN GARDNER wrote:
>> 
>> 
>>> ________________________________
>>> From: Michael Niedermayer <michaelni at gmx.at>
>>> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org> 
>>> Sent: Monday, 16 September 2013, 18:09
>>> Subject: Re: [FFmpeg-devel] Small modifcation to libavformat/dvbsubdec.c
>>> 
>>> 
>>> On Mon, Sep 16, 2013 at 08:46:39AM +0100, JULIAN GARDNER wrote:
>>>> 
>>>> 
>>>>> ________________________________
>>>>> From: JULIAN GARDNER <joolzg at btinternet.com>
>>>>> To: FFmpegdevelopment discussions and patches <ffmpeg-devel at ffmpeg.org> 
>>>>> Sent: Wednesday, 4 September 2013, 23:51
>>>>> Subject: Small modifcation to libavformat/dvbsubdec.c
>>>>> 
>>>>> 
>>>>> 
>>>>> diff --git a/libavcodec/dvbsubdec.c b/libavcodec/dvbsubdec.c
>>>>> index 955925a..faa510a 100644
>>>>> --- a/libavcodec/dvbsubdec.c
>>>>> +++ b/libavcodec/dvbsubdec.c
>>>>> @@ -1015,9 +1015,9 @@ static void dvbsub_parse_clut_segment(AVCodecContext *avctx,
>>>>>  
>>>>>          if (depth & 0x80)
>>>>>              clut->clut4[entry_id] = RGBA(r,g,b,255 - alpha);
>>>>> -        if (depth & 0x40)
>>>>> +        else if (depth & 0x40)
>>>>>              clut->clut16[entry_id] = RGBA(r,g,b,255 - alpha);
>>>>> -        if (depth &
>>>> 0x20)
>>>>> +        else if (depth & 0x20)
>>>>>              clut->clut256[entry_id] = RGBA(r,g,b,255 - alpha);
>>>>>      }
>>>>>      }
>>>>> 
>>>>> 
>>>>> Just so it adheres to spec. Page 25 of ETS300743 V1.2.1, page 25.
>>>>> 
>>>>> 2-bit/entry_CLUT_flag: If set to '1', this indicates that this CLUT value is to be loaded into the identified entry of the
>>>> 2-bit/entry CLUT.  
>>>>> 4-bit/entry_CLUT_flag: If set to '1', this indicates that this CLUT value is to be loaded into the identified entry of the
>>>> 4-bit/entry CLUT. 
>>>>> 8-bit/entry_CLUT_flag: If set to '1', this indicates that this CLUT value is to be loaded into the identified entry of the
>>>> 8-bit/entry CLUT. 
>>>>> NOTE 1: Only one N-bit/entry_CLUT_flag shall be set to 1 per CLUT_entry_id and its associated Y-, Cr-, Cb- and
>>>> T -values. 
>>>>> joolz
>>>>> 
>>>> Any comment on this, will it be pushed?
>>> 
>>> what does this patch fix ?
>>> please provide a testcase or if it fixes spec compliance a quote of
>>> the spec saying that if multiple bits are set this new behavior is
>>> to be used.
>>> 
>>> You quote just a part of the spec that says that the affected case
>>> shall not occur. why would a change in behavior for a case that lies
>>> outside the spec be a matter of spec adherence ?
>>> I surely am missing something here ...
>>> 
>>> [...]
>>> -- 
>>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>> 
>>> No human being will ever know the Truth, for even if they happen to say it
>>> by chance, they would not even known they had done so. -- Xenophanes
>>> 
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> This patch fixes a non corfmance to the DVB Subtitle spectications, as i pointed out in my mail.
>> 
>> No i cannot provide a non working one as this would break the specification. All DVB Streams adhere to this specification, find me one that fails and I will bow to your interpretation.
>> 
>> 
>> I think English is not you mother tongue but 
>> 
>> 
>> "NOTE 1: Only one N-bit/entry_CLUT_flag SHALL be set to 1 per CLUT_entry_id and its associated Y-, Cr-, Cb- and T -values." 
>> 
>> 
>> SHALL (from dictionary.com)
>> 3.(in laws, directives, etc.) must; is or are obliged to: The meetings of the council shall be public.  
>> 
>> OBLIGED (from dictionary.com)
>> 1.to require or constrain, as by law, command, conscience, or force of necessity.
>> 
>> 
>> CONSTRAIN (from dictionary.com)
>> 1. to force, compel, or oblige
>> 
>> 
>> To a native english speaker this is clear, You can only set a one of the 3 bits.
>> 
>> 
>> So its either ffmpeg follows spec, or ffmpeg follows Michaels interpretation of the spec.
> 
> Can someone else please look at the patch and the quoted text from the
> spec and comment if this patch is correct and fixes a spec non
> conformance issue or not.

Sorry, but I follow your conclusion: the specification to me seems to clearly say that any file/stream where this change makes a difference is broken and non-conformant.
It might make sense to make the code print a warning for that case though, then we'll at least know when it happens.