[FFmpeg-devel] [PATCH] HAM6/HAM8 support for IFF demuxer/decoder

Sebastian Vater cdgs.basty
Tue May 4 21:13:46 CEST 2010


Sebastian Vater a ?crit :
> Sebastian Vater a ?crit :
>   
>> Peter Ross a ?crit :
>>   
>>     
>>> On Thu, Apr 29, 2010 at 02:11:25PM +0200, Sebastian Vater wrote:
>>>   
>>>     
>>>       
>>>> Michael Niedermayer a ?crit :
>>>>     
>>>>       
>>>>         
>>>>> On Thu, Apr 29, 2010 at 01:51:09PM +0200, Sebastian Vater wrote:
>>>>>   
>>>>>       
>>>>>         
>>>>>           
>>>>>> Michael Niedermayer a ?crit :
>>>>>>     
>>>>>>         
>>>>>>           
>>>>>>             
>>>>>>> On Thu, Apr 29, 2010 at 01:10:18PM +0200, Sebastian Vater wrote:
>>>>>>>   
>>>>>>>       
>>>>>>>           
>>>>>>>             
>>>>>>>               
>>>>>>>> Peter Ross a ?crit :
>>>>>>>>     
>>>>>>>>         
>>>>>>>>             
>>>>>>>>               
>>>>>>>>                 
>>>>>>>>> On Wed, Apr 28, 2010 at 11:17:12PM +0200, Sebastian Vater wrote:
>>>>>>>>>   
>>>>>>>>>       
>>>>>>>>>           
>>>>>>>>>               
>>>>>>>>>                 
>>>>>>>>>                   
>>>>>>>>>> Sebastian Vater a ?crit :
>>>>>>>>>>     
>>>>>>>>>>         
>>>>>>>>>>             
>>>>>>>>>>                 
>>>>>>>>>>                   
>>>>>>>>>>                     
>>>>>>>>>>> So here is it!
>>>>>>>>>>>
>>>>>>>>>>> The long awaited HAM6/8 decoding support for IFF-ILBM.
>>>>>>>>>>>       
>>>>>>>>>>>           
>>>>>>>>>>>               
>>>>>>>>>>>                   
>>>>>>>>>>>                     
>>>>>>>>>>>                       
>>>>>>>>> Yep, glad somebody took the time.
>>>>>>>>>
>>>>>>>>>   
>>>>>>>>>       
>>>>>>>>>           
>>>>>>>>>               
>>>>>>>>>                 
>>>>>>>>>                   
>>>>>>>>>> +#define CAMG_EHB  0x80   // Extra HalfBrite
>>>>>>>>>>     
>>>>>>>>>>         
>>>>>>>>>>             
>>>>>>>>>>                 
>>>>>>>>>>                   
>>>>>>>>>>                     
>>>>>>>>> Macro isn't used.
>>>>>>>>>   
>>>>>>>>>       
>>>>>>>>>           
>>>>>>>>>               
>>>>>>>>>                 
>>>>>>>>>                   
>>>>>>>> Just added it, the next patch for EHB support will use it, should I
>>>>>>>> remove it until then and add the define when EHB is done?
>>>>>>>>
>>>>>>>>     
>>>>>>>>         
>>>>>>>>             
>>>>>>>>               
>>>>>>>>                 
>>>>>>>>>   
>>>>>>>>>       
>>>>>>>>>           
>>>>>>>>>               
>>>>>>>>>                 
>>>>>>>>>                   
>>>>>>>>>> @@ -152,6 +157,12 @@ static int iff_read_header(AVFormatContext *s,
>>>>>>>>>>              st->codec->channels = (get_be32(pb) < 6) ? 1 : 2;
>>>>>>>>>>              break;
>>>>>>>>>>  
>>>>>>>>>> +        case ID_CAMG:
>>>>>>>>>> +            if (data_size < 4)
>>>>>>>>>> +                return AVERROR_INVALIDDATA;
>>>>>>>>>>     
>>>>>>>>>>         
>>>>>>>>>>             
>>>>>>>>>>                 
>>>>>>>>>>                   
>>>>>>>>>>                     
>>>>>>>>> Is this really neccessary. We dont check data_size for any of the other tags.
>>>>>>>>>   
>>>>>>>>>       
>>>>>>>>>           
>>>>>>>>>               
>>>>>>>>>                 
>>>>>>>>>                   
>>>>>>>> The IFF standard says that chunks can be of any length, it seems that
>>>>>>>> you missed by patch which does add the checking for other tags, too.
>>>>>>>>
>>>>>>>> Not checking such stuff could also cause security issues if someone with
>>>>>>>> a bad mind prepares an attack with a malformed IFF file.
>>>>>>>>
>>>>>>>> It would not be the first case picture decoders cause buffer overflows
>>>>>>>> thanks to malformed image data/structure. ;-)
>>>>>>>>
>>>>>>>> Also chunks can always be longer than the initial definition to it, just
>>>>>>>> like you can add new data fields to a struct. ;-)
>>>>>>>>
>>>>>>>> But in one point you're correct here, it should really be evaluated if
>>>>>>>> the decoder should abort if the chunk is to small (i.e. just 2 bytes,
>>>>>>>> because the amiga display mode used for it had a zero lower word) or
>>>>>>>> just consider to small stuff to be set to 0 (in that case non-HAM and
>>>>>>>> non-EHB would always be assumed).
>>>>>>>>
>>>>>>>>     
>>>>>>>>         
>>>>>>>>             
>>>>>>>>               
>>>>>>>>                 
>>>>>>>>>> +        if (iff->camg_display_mode & CAMG_HAM) {
>>>>>>>>>> +            switch (st->codec->bits_per_coded_sample) {
>>>>>>>>>> +            case 6: // HAM6
>>>>>>>>>> +                st->codec->bits_per_coded_sample = 12; // 4096 color HAM6 image
>>>>>>>>>> +                break;
>>>>>>>>>> +
>>>>>>>>>> +            case 8: // HAM8
>>>>>>>>>> +                st->codec->bits_per_coded_sample = 18; // 262144 color HAM8 image
>>>>>>>>>> +                break;
>>>>>>>>>>     
>>>>>>>>>>         
>>>>>>>>>>             
>>>>>>>>>>                 
>>>>>>>>>>                   
>>>>>>>>>>                     
>>>>>>>>> My only concern here is that 'bits_per_coded_sample' is being abused to
>>>>>>>>> indicate both bpp *and* whether HAM coding is used. If somebody out there
>>>>>>>>> encodes 12-bit RGB into IFF then we will have a problem.
>>>>>>>>>   
>>>>>>>>>       
>>>>>>>>>           
>>>>>>>>>               
>>>>>>>>>                 
>>>>>>>>>                   
>>>>>>>> I just realized that too.
>>>>>>>>
>>>>>>>> And I'm just thinking of a better solution. Is it appreciated in ffmpeg
>>>>>>>> to use codec_tag as a bitfield or split up the uint32_t internally into
>>>>>>>> 4 uint8_t or 2 2 uint8_t's?
>>>>>>>>
>>>>>>>> Right now the IFF demuxer and decoder use codec_tag to store whether
>>>>>>>> it's an PBM or ILBM file.
>>>>>>>>
>>>>>>>> I would like to split it internally into 4 bytes and use one byte for
>>>>>>>> tagging if it's EHB/HAM, one byte for transparent color index, one byte
>>>>>>>> for the compression identifier and one byte for plane number being a
>>>>>>>> mask to skip.
>>>>>>>>     
>>>>>>>>         
>>>>>>>>             
>>>>>>>>               
>>>>>>>>                 
>>>>>>> you would like to look at AVCodecContext.extradata
>>>>>>>
>>>>>>> [...]
>>>>>>>   
>>>>>>>       
>>>>>>>           
>>>>>>>             
>>>>>>>               
>>>>>> I know extradata, but it's used for the palette data already and moving
>>>>>> that data structures around or adding sth. at the end requires defining
>>>>>> a completely new structure (one for the palette data and one for extra
>>>>>> data).
>>>>>>
>>>>>> So I'ld use codec_tag for it, using an whole 32 bit integer just for
>>>>>> indicating if it's a PBM or ILBM (one bit would be enough for that) is
>>>>>> in my eyes, just a waste.
>>>>>>
>>>>>> Or are there any reasons for not doing it the way, I wrote my last msg?
>>>>>>     
>>>>>>         
>>>>>>           
>>>>>>             
>>>>> dont forget to fill AVIn/OutputFormat.codec_tag ;)
>>>>> (when you try you will see why the idea of spliting codec_tag like this
>>>>>  doesnt work)
>>>>>   
>>>>>       
>>>>>         
>>>>>           
>>>> Oh ok, I got the point. So it should always be a kind of fourcc, right?
>>>>     
>>>>       
>>>>         
>>> Yes, its meant to be a 'tag' for identification.
>>>
>>>   
>>>     
>>>       
>>>> Then I have to do this with extradata...
>>>>     
>>>>       
>>>>         
>>> Yep, just allocate a fixed length of bytes (for the flags) at the start
>>> of the extradata buffer and then append palette.
>>>   
>>>     
>>>       
>> Fixed! Please review attached patch.
>>   
>>     
>
> Just figured out that the last patch doesn't decode HAM5/7 properly.
>
> Now it does. The error was  not setting hbits correctly (should always
> be 4 or 6, even for HAM5 and HAM7).
>
> Besides this I fixed an indentation of actually new code.
>   

Damn, this can't be true!

Now  I just forgot to free the HAM buffers in decode_end, so here is
another corrected patch.

Also I detected and removed a unneeded memset in IFF-ILBM HAM decoder.

Sorry for this!

-- 

Best regards,
                   :-) Basty/CDGS (-:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: iff-ham-support.patch
Type: text/x-patch
Size: 11080 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100504/f698a89b/attachment.bin>



More information about the ffmpeg-devel mailing list