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

Sebastian Vater cdgs.basty
Sun May 9 21:02:21 CEST 2010

Martin Storsj? a ?crit :
> On Sun, 9 May 2010, Sebastian Vater wrote:
> I agree with Ronald, this feels excessive. Adjusting extradata size so 
> that multiplying/dividing by it is simple is just premature optimization, 
> which is the root of all evil.

After all the structure should be long enough so it will very probably
never be extended anymore, and allocating 250 bytes more shouldn't kill
the universe. ;-)

But if you dare, I will change this to a value lesser than 1KB...

>> Because I use this to calculate the palette size (in fact the original
>> code did this)...by adding data_size to IFF_EXTRA_CONTEXT_SIZE I just
>> have to subtract IFF_EXTRA_CONTEXT_SIZE to get the actual palette size.
>> And no, I can't because of compatibility reasons, put a number of
>> palette entries before the palette data.
> Can't you in this case include the count in one of the bytes later in the 
> extradata?

And how does an old lavc/iff.c differ then if data is really coming from
a new version lavf/iff.c?
I tried that out, but it won't work. And how do I find the count when
it's on a variable position?
I'm afraid, that it will cause much more hassle than the current version.
Also I can't use tags there, because the 3-pair palette data is from
0-255 each, so any of the 16777216 possible values is possible as actual
palette data.

> I'm not totally familiar with the rest of the code, but instead of 
> including the maximum palette within IFF_EXTRA_CONTEXT_SIZE, couldn't you 
> signal the number of palette entries in the same way as now, count = 1 << 
> avctx->bits_per_coded_sample, and then just look for the new extradata 
> after 3 * count bytes? Or perhaps bits_per_coded_sample is set in some 
> other way now?

This is a very bad idea, because the chunk length can be odd if the
chunk contains only 31 entries (which is perfectly legal in
IFF-ILBM)...since the data structure contains an uint16_t, this will
slow down and even crash on any CPU which can't access such data on an
odd address (like the original 68000, for example).

>> +    if (s->ham) { // adjust HAM color palette
>> +        count = 1 << s->ham;
>> +    } else if (s->ehb) { // EHB (ExtraHalfBrite) color palette
>> +        av_log(avctx, AV_LOG_ERROR, "ExtraHalfBrite (EHB) mode not supported\n");
>> +        return AVERROR_PATCHWELCOME;
>> +    } else {
>> +        count = 1 << avctx->bits_per_coded_sample;
>>      }
>> +    if (avctx->extradata_size >= IFF_EXTRA_CONTEXT_SIZE) {
>> +        count = FFMIN((avctx->extradata_size - IFF_EXTRA_CONTEXT_SIZE) / 3, count);
>> +    } else {
>> +        count = FFMIN(avctx->extradata_size / 3, count);
>> +    }
> Why are you first setting count to something according to one formula, and 
> then after that setting it to something else (or the same?) calculated in 
> some other way?

Because an IFF file can have CMAP chunk data smaller than this. Removing
the FFMIN stuff will break sabre.iff which just does this. That's also
why I removed palette underflow error message.
If you are meaning the if (avctx->extradata_size >=
IFF_EXTRA_CONTEXT_SIZE), it is necessary for backward compatibility, I
found out today it won't work as expected without the IFF line.
If the new lavc code is called from old lavf it will show ANY non-HAM
image as greyscale.

>> @@ -229,6 +248,22 @@ static int iff_read_header(AVFormatContext *s,
>>      url_fseek(pb, iff->body_pos, SEEK_SET);
>> +    if (!st->codec->extradata) {
>> +        st->codec->extradata_size = IFF_EXTRA_CONTEXT_SIZE;
>> +        st->codec->extradata = av_malloc(IFF_EXTRA_CONTEXT_SIZE + FF_INPUT_BUFFER_PADDING_SIZE);
>> +        if (!st->codec->extradata)
>> +            return AVERROR(ENOMEM);
>> +    }
>> +    ex = GET_EXTRA_CONTEXT(st->codec);
>> +    memset(ex, 0, IFF_EXTRA_CONTEXT_SIZE);
> The case where the compatibility palette in extradata is needed is when 
> you don't get any CMAP, but you want to add the new flags in extradata, 
> right?. In that case, you simply memset the cmap part to 0. Does this 
> really do what you intended, with an older libavcodec/iff.c - wouldn't you 
> have to initialize the palette part of IFF_EXTRA_CONTEXT_SIZE to a gray 
> palette or something similar?

I tested this very carefully today with both old lavf/iff.c calling new
lavc/iff.c and vice versa and it works like a charm with the method I'm
using right now!
However this doesn't work anymore if IFF_EXTRA_CONTEXT_SIZE is smaller
or equal than the largest possible CMAP value in the old demuxer/decoder
(256*3 = 768 bytes).


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

More information about the ffmpeg-devel mailing list