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

[Ronald S. Bultje]

Hi,

On Sat, May 8, 2010 at 7:07 AM, Sebastian Vater
<cdgs.basty at googlemail.com> wrote:
> Apart from this, it's pretty the same, hope this patch is ok, so we can
> finally add it to git/svn to have full HAM support ;)
[..]
> +    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);

There's no reason to use GET_EXTRA_CONTEXT() here, since allocation is
always large enough (and you don't check for NULL anyway).

Then...:

> +    const uint8_t *ex = GET_EXTRA_CONTEXT(avctx);

Only use of GET_EXTRA_CONTEXT(), so you can now easily integrate that
macro in this function and remove it.

> +/**
> + * IFF extra context size has to be larger than maximum palette data
> + * size of previous IFF demuxer and decoder in order to retain
> + * backward compatibility between an old version of IFF decoder and
> + * current version of IFF demuxer.
> + *
> + * Since 256 palette entries with 3 bytes each color yields in
> + * 768 bytes, we simply round up to nearest 512 byte block, which
> + * is exactly 1 KB.
> + */
> +#define IFF_EXTRA_CONTEXT_SIZE      1024

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

> +// Gray to RGB, required for HAM palette table
> +#define GRAY2RGB(x) (((x) << 16) | ((x) <<  8) | (x))

Can the decoder output GRAY8 instead of RGB24 for these frames (or are
some frames this mode, and others RGB?). Also, this is only used once.

> +// screenmode bits
> +#define CAMG_EHB 0x80    // Extra HalfBrite
> +#define CAMG_HAM 0x800   // Hold And Modify
> +
> +// TODO: masking bits
> +#define MASK_NONE                  0
> +#define MASK_HAS_MASK              1
> +#define MASK_HAS_TRANSPARENT_COLOR 2
> +#define MASK_LASSO                 3

These (except MASK_NONE) are only used in the decoder and can thus be
moved out of the header file. I wouldn't mind if you moved MASK_NONE
also and simply changed the demuxer to init at 0 instead of MASK_NONE,
with a comment.

> +    uint8_t   compression;
> +    uint8_t   ham;
> +    uint8_t   ehb;
> +    uint8_t   masking;
> +    uint16_t  transparency;
>  } IffContext;

Generally, code is faster if you make this native type width, i.e. just int.

> @@ -290,7 +313,7 @@ static int iff_read_packet(AVFormatContext *s,
>      if(iff->sent_bytes >= iff->body_size)
>          return AVERROR(EIO);
>
> -    if(s->streams[0]->codec->channels == 2) {
> +    if(st->codec->channels == 2) {
>          uint8_t sample_buffer[PACKET_SIZE];
>
>          ret = get_buffer(pb, sample_buffer, PACKET_SIZE);

This belongs in its own patch, same for the other ones later in this file.

> @@ -256,13 +285,7 @@ static int iff_read_header(AVFormatContext *s,
>      case AVMEDIA_TYPE_VIDEO:
>          switch (compression) {
>          case BITMAP_RAW:
> -            if (st->codec->codec_tag == ID_ILBM) {
>                  st->codec->codec_id = CODEC_ID_IFF_ILBM;
> -            } else {
> -                st->codec->codec_id = CODEC_ID_RAWVIDEO;
> -                st->codec->pix_fmt  = PIX_FMT_PAL8;
> -                st->codec->codec_tag = 0;
> -            }
>              break;
>          case BITMAP_BYTERUN1:
>              st->codec->codec_id = CODEC_ID_IFF_BYTERUN1;
> @@ -299,19 +322,7 @@ static int iff_read_packet(AVFormatContext *s,
>              return AVERROR(ENOMEM);
>          }
>          interleave_stereo(sample_buffer, pkt->data, PACKET_SIZE);
> -    } else if (s->streams[0]->codec->codec_id == CODEC_ID_RAWVIDEO) {
> -        if(av_new_packet(pkt, iff->body_size + AVPALETTE_SIZE) < 0) {
> -            return AVERROR(ENOMEM);
> -        }
> -
> -        ret = ff_cmap_read_palette(st->codec, (uint32_t*)(pkt->data + iff->body_size));
> -        if (ret < 0)
> -            return ret;
> -        av_freep(&st->codec->extradata);
> -        st->codec->extradata_size = 0;
> -
> -        ret = get_buffer(pb, pkt->data, iff->body_size);
> -    } else if (s->streams[0]->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
> +    } else if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
>          ret = av_get_packet(pb, pkt, iff->body_size);
>      } else {
>          ret = av_get_packet(pb, pkt, PACKET_SIZE);

So now you're adding "codec handling" for PBM data, right? Is that
intended? I'd guess that decoding is faster with rawvideo decoder than
with an actual decoder here.

Regardless, this should be its own patch.

>              st->codec->height                = get_be16(pb);
>              url_fskip(pb, 4); // x, y offset
>              st->codec->bits_per_coded_sample = get_byte(pb);
> -            if (data_size >= 11) {
> -                url_fskip(pb, 1); // masking
> +            if (data_size >= 10)
> +                masking                      = get_byte(pb);
> +            if (data_size >= 11)
>                  compression                  = get_byte(pb);
> +            if (data_size >= 14) {
> +                url_fskip(pb, 1); // padding
> +                transparency                 = get_be16(pb);
>              }
>              if (data_size >= 16) {
> -                url_fskip(pb, 3); // paddding, transparent
>                  st->sample_aspect_ratio.num  = get_byte(pb);
>                  st->sample_aspect_ratio.den  = get_byte(pb);
>              }

Do files with "random" sizes exist? I'd assume that this block either
always has >=16 bytes, or 10 bytes (no masing/compress/etc). You could
make this block simpler then.

> -            st->codec->extradata_size = data_size;
[..]
> +            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?

> +    const unsigned hbits = bps > 6 ? 6 : 4;
[..]
> +        switch (v >> hbits) {
> +        case 0: // take direct color value from palette
[..]
> +        case 1: // just modify blue color component
[..]
> +        case 2: // just modify red color component
[..]
> +        case 3: // just modify green color component
[..]
> +    }

What if bps<6 and v>>hbits is 4-15? You want ot at least document what
happens then, or emit an error if it shouldn't.

Ronald