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

[Martin Storsjö]

On Sun, 9 May 2010, Sebastian Vater wrote:

> > Why round to the nearest 512-byte block? It seems to me that
> > extradata_size is going to be used for decoder versioning, so you
> > wouldn't want to round this, but heave it be exact for all data
> > expected in here (so 774 bytes, not 1024).
> >   
> 
> Only for differentiating the old versions of decoder, newer versions
> won't increase that anymore, as I described in the header file, I'll
> using non 0 tags for it...
> Why did I choose 1 KB? It's just a nicer value than 774 and if we ever
> need to multiply / divide by it, we can replace these with shifting
> instructions.

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.

> > [..]
> >   
> >> +            st->codec->extradata_size = data_size + IFF_EXTRA_CONTEXT_SIZE;
> >>     
> >
> > data_size is the palette size, right? So why is palette_size included
> > in IFF_EXTRA_CONTEXT_SIZE also?
> >   
> 
> 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?

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?

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


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

// Martin