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

[Sebastian Vater]

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.

>> +    } else if (s->masking != MASK_NONE) {
>> +        av_log(avctx, AV_LOG_ERROR, "Masking not supported\n");
>> +        return AVERROR_INVALIDDATA;
>> +    } else if ((s->masking == MASK_HAS_TRANSPARENT_COLOR) && (s->transparency != 0)) {
>> +        av_log(avctx, AV_LOG_ERROR, "Transparency not supported\n");
>> +        return AVERROR_INVALIDDATA;
>>      }
>>     
>
> return AVERROR_PATCHWELCOME.
>   

Fixed.

>> -    count = 1 << avctx->bits_per_coded_sample;
>> -    if (avctx->extradata_size < count * 3) {
>> -        av_log(avctx, AV_LOG_ERROR, "palette data underflow\n");
>> +    if (s->ham) { // adjust HAM color palette
>> +        count = 1 << (avctx->bits_per_coded_sample > 6 ? 6 : 4); // adjust HAM5/HAM7 to HAM6/HAM8
>> +    } else if (s->ehb) { // EHB (ExtraHalfBrite) color palette
>> +        av_log(avctx, AV_LOG_ERROR, "ExtraHalfBrite (EHB) mode not supported\n");
>>          return AVERROR_INVALIDDATA;
>>     
>
> AVERROR_PATCHWELCOME
>   

Fixed.

>> +        if (count != 0) { // HAM with normal color palette
>> +            s->ham_palbuf = av_malloc((count * sizeof (uint32_t)) + FF_INPUT_BUFFER_PADDING_SIZE);
>> +            if (!s->ham_palbuf)
>> +                return AVERROR(ENOMEM);
>> +            for (i=0; i < count; i++) {
>> +                s->ham_palbuf[i] = AV_RL24( avctx->extradata + i*3 );
>> +            }
>> +        } else { // HAM with grayscale color palette
>> +            count = 1 << (avctx->bits_per_coded_sample > 6 ? 6 : 4);
>> +            s->ham_palbuf = av_malloc((count * sizeof (uint32_t)) + FF_INPUT_BUFFER_PADDING_SIZE);
>> +            if (!s->ham_palbuf)
>> +                return AVERROR(ENOMEM);
>>     
>
> Isn't this leaking? If this fails s->ham_buf is not freed.
>   

Fixed.

>> +/**
>> + * Convert one line of HAM6/8-encoded chunky buffer to 24bpp
>>     
>
> third person -> Converts -> for bikeshed sake
>   

Fixed.

>   
>> + * @param dst Destination 24bpp buffer
>>     
>
> the destination
>   

Fixed.

>   
>> + * @param buf Source 8bpp chunky buffer
>>     
>
> the source
>   

Fixed.

>> + * @param pal lavc palette with alpha channel always at 0
>> + * @param buf_size
>> + * @param bps 5-6 for HAM6, 7-8 for HAM8
>> + */
>> +
>>     
>
> Ugly empty newline.
>   

Fixed.

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

>>      switch(st->codec->codec_type) {
>>      case AVMEDIA_TYPE_AUDIO:
>>          av_set_pts_info(st, 32, 1, st->codec->sample_rate);
>> @@ -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;
>> @@ -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) {
>>     
>
> cosmetics -> separate patch
>   

Fixed.

>> -    } else if (s->streams[0]->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
>> +    } else if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
>>     
>
> ditto
>   

Fixed.

>>          ret = av_get_packet(pb, pkt, iff->body_size);
>>      } else {
>>          ret = av_get_packet(pb, pkt, PACKET_SIZE);
>> @@ -320,13 +331,13 @@ static int iff_read_packet(AVFormatContext *s,
>>      if(iff->sent_bytes == 0)
>>          pkt->flags |= AV_PKT_FLAG_KEY;
>>  
>> -    if(s->streams[0]->codec->codec_type == AVMEDIA_TYPE_AUDIO) {
>> +    if(st->codec->codec_type == AVMEDIA_TYPE_AUDIO) {
>>     
>
> ditto
>   

Fixed.

>>          iff->sent_bytes += PACKET_SIZE;
>>      } else {
>>          iff->sent_bytes = iff->body_size;
>>      }
>>      pkt->stream_index = 0;
>> -    if(s->streams[0]->codec->codec_type == AVMEDIA_TYPE_AUDIO) {
>> +    if(st->codec->codec_type == AVMEDIA_TYPE_AUDIO) {
>>     
>
> ditto
>   

Fixed.

>>          pkt->pts = iff->audio_frame_count;
>>          iff->audio_frame_count += ret / s->streams[0]->codec->channels;
>>      }
>>     
-- 

Best regards,
                   :-) Basty/CDGS (-:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: iff-ham-support.patch
Type: text/x-patch
Size: 20400 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100509/51654ce6/attachment.bin>