[FFmpeg-devel] [PATCH] HAM6/HAM8 support for IFF demuxer/decoder
Sebastian Vater
cdgs.basty
Tue May 4 20:59:39 CEST 2010
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.
--
Best regards,
:-) Basty/CDGS (-:
-------------- next part --------------
A non-text attachment was scrubbed...
Name: iff-ham-support.patch
Type: text/x-patch
Size: 10868 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100504/c644e199/attachment.bin>
More information about the ffmpeg-devel
mailing list