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

[Sebastian Vater]

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?

-- 

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