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

Michael Niedermayer michaelni
Thu Apr 29 13:19:16 CEST 2010


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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/c430e941/attachment.pgp>



More information about the ffmpeg-devel mailing list