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

[Stefano Sabatini]

On date Sunday 2010-05-09 18:17:50 +0200, Sebastian Vater encoded:
> Stefano Sabatini a ?crit :
> > On date Thursday 2010-05-06 23:23:20 +0200, Sebastian Vater encoded:
> >   
> >> Sebastian Vater a ?crit :
> >>     
> >>  /**
> >>   * Convert CMAP buffer (stored in extradata) to lavc palette format
> >>   */
> >> -int ff_cmap_read_palette(AVCodecContext *avctx, uint32_t *pal)
> >> +static int cmap_read_palette(AVCodecContext *avctx, uint32_t *pal)
> >>     
> >
> > This looks fine, but this change deserves a separate patch.
> >   
> 
> Why? It's important to change this with HAM support, the old code
> doesn't really have any problems with it.

Looks orthogonal to the current change, you could just keep it
non static with the new code too, right? So I don't see why it is
required by *this* change (but I may have missed something from the
previous discussion).
 
[...]

> >> +            for(x = 0; x < avctx->width && buf < buf_end; ) {
> >> +                const int8_t value = *buf++;
> >> +                unsigned length;
> >> +                if (value >= 0) {
> >> +                    length = value + 1;
> >> +                    memcpy(s->ham_buf + x, buf, FFMIN3(length, buf_end - buf, avctx->width - x));
> >> +                    buf += length;
> >> +                } else if (value > -128) {
> >> +                    length = -value + 1;
> >> +                    memset(s->ham_buf + x, *buf++, FFMIN(length, avctx->width - x));
> >> +                } else { // noop
> >> +                    continue;
> >> +                }
> >> +                x += length;
> >> +            }
> >>     
> >
> > Looks similar to "HAM to PIX_FMT_BGR32", maybe it can be factorized.
> >   
> 
> Would add an unnecessary if in a loop, for sake of perfomance, I decided
> to do this as an extra part.
>
> Or maybe you want me having a #define for it?

Yes a macro should be fine. 

[...]

> diff --git a/libavcodec/iff.c b/libavcodec/iff.c
> index e563de1..065c833 100644
> --- a/libavcodec/iff.c
> +++ b/libavcodec/iff.c
[...]
> @@ -166,6 +243,55 @@ static void decodeplane32(uint32_t *dst, const uint8_t *const buf, int buf_size,
>      }
>  }
>  
> +/**
> + * Converts one line of HAM6/8-encoded chunky buffer to 24bpp
> + * @param dst the destination 24bpp buffer

Nit++: an empty line before @params helps readability.

> + * @param buf the source 8bpp chunky buffer
> + * @param pal lavc palette with alpha channel always at 0
> + * @param buf_size
> + * @param hbits 4 for HAM5/HAM6, 6 for HAM7/HAM8
> + */
> +static void decodehamplane32(uint32_t *dst,
> +                             const uint8_t  *buf,
> +                             const uint32_t *const pal,
> +                             const unsigned buf_size,
> +                             const unsigned hbits)
[...]

> diff --git a/libavcodec/iff.h b/libavcodec/iff.h
> index 76db10b..ff01f2a 100644
> --- a/libavcodec/iff.h
> +++ b/libavcodec/iff.h
[...]
> @@ -256,13 +291,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,18 +328,6 @@ 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);

Could you explain these last two chunks? I see no obvious correlation
with the rest of the patch.

Regards.
-- 
FFmpeg = Friendly & Free Majestic Pure Enchanting Gymnast