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

[Peter Ross]

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.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100429/7f14cbd1/attachment.pgp>