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

[Sebastian Vater]

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 (-: