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

[Martin Storsjö]

On Sun, 9 May 2010, Sebastian Vater wrote:

> 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. ;-)

250 bytes extra sure feels excessive, but wait for comments from Ronald or 
someone else, too.

> >> 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?

An old lavc/iff.c only calculates
   count = 1 << avctx->bits_per_coded_sample;
and only checks that extradata_size is large enough, that's the only place 
where extradata_size is used currently.

> 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).

No, it won't crash on such CPUs. You're using bytestream_get_be16 and 
bytestream_put_be16 for reading/writing, and they're using AV_RB16/AV_WB16 
internally, which do allow unaligned access.

Is there any other reason except for this?

> >   
> >> +    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?

Nevermind, I didn't notice that the earlier count value was used in the 
later calculation.

> 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.

In which way will it break? Wouldn't it just populate the palette with 
some colors which would be 0,0,0, which would never be used?

> >> @@ -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!

Ok, so in that case, you only need the 768 first bytes for lavc/iff.c to 
recognize that this is the new format, their value will never be used 
unless they're actually populated with data from a cmap?

I still don't like that you include a full palette in 
IFF_EXTRA_CONTEXT_SIZE, which is never used, even if you actually get a 
CMAP that you pass there.

Couldn't you do something like this, in pseudocode:

#define IFF_EXTRA_CONTEXT_SIZE 20 (or whatever, the currently actually 
used values + some space for growing later)

lavf/iff_read_header:
	case ID_CMAP:
		if (data_size > 768)
			error?
		extradata_size = 768 + IFF_EXTRA_CONTEXT_SIZE
		copy actual data_size into extradata
		store data_size somewhere in IFF_EXTRA_CONTEXT_SIZE

	if no extradata, no cmap encountered
		extradata_size = 768 + IFF_EXTRA_CONTEXT_SIZE

lavc/decode_init:
	if extradata_size >= 768 + IFF_EXTRA_CONTEXT_SIZE
		handle as new format
	else
		handle as old format

That feels like much less duplication to me.

Also, I'm not sure you really need to store the original CMAP data_size 
within IFF_EXTRA_CONTEXT_SIZE, couldn't you just convert all palette 
entries, so that you'd get extra black palette entries at the end, that 
aren't used - like I said above?

// Martin