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

Sebastian Vater cdgs.basty
Thu Apr 29 13:10:18 CEST 2010


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.

The decoder then "unpacks" the 4 byte structure into IffContext to
appreciate int values.

Mask planes are usually used in sprite editors which save IFF-ILBM files
or DPaint saved brushes. Not handling mask plane properly will cause
images which have a stencil attached to be displayed completely incorrect.
(Mask plane is simply a plane which OR's together all non-mask planes,
thus for any pixel having color value != 0x000000 the mask plane will
have it's pixel set, too).

The Amiga uses this also for drawing transparent images on a prefilled
screen by using the blitter in XOR mode with the destination bitmap, the
source bitmap and the mask plane.

If you don't have any complaints about this approach, I will create a
new patch which does it this way.

-- 

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




More information about the ffmpeg-devel mailing list