[FFmpeg-devel] [PATCH] correctly handle MSRLE uncompressed

[Kostya]

On Tue, May 26, 2009 at 11:51:08PM -0700, Baptiste Coudurier wrote:
> On 5/26/2009 10:01 PM, Kostya wrote:
> > On Tue, May 26, 2009 at 09:00:41PM -0700, Baptiste Coudurier wrote:
> >> Hi Kostya,
> >>
> >> On 5/24/2009 3:17 AM, Kostya wrote:
> >>>> [...]
> >>>>
> >>>> @@ -88,8 +101,31 @@
> >>>>          }
> >>>>      }
> >>>>  
> >>>> -    ff_msrle_decode(avctx, (AVPicture*)&s->frame, avctx->bits_per_coded_sample, buf, buf_size);
> >>>> +    /* FIXME how to correctly detected RLE ??? */
> >>> how to correctly detect RLE
> >>> Hint: escape codes at the end
> >> Humm, like 0 1 ?
> >>
> >>>> +    if (avctx->height * istride == avpkt->size) {
> >>>> +        int linesize = avctx->width * avctx->bits_per_coded_sample / 8;
> >>>> +        uint8_t *ptr = s->frame.data[0];
> >>>> +        uint8_t *buf = avpkt->data + (avctx->height-1)*istride;
> >>>> +        int i, j;
> >>>>  
> >>>> +        for (i = 0; i < avctx->height; i++) {
> >>>> +            if (avctx->bits_per_coded_sample == 4) {
> >>>> +                for (j = 0; j < linesize; j++) {
> >>>> +                    ptr[j*2+0] = buf[j] >> 4;
> >>>> +                    ptr[j*2+1] = buf[j] & 0xf;
> >>>> +                }
> >>>> +                if (j*2 < avctx->width)
> >>>> +                    ptr[j*2+0] = buf[j] >> 4;
> >>> I'd rather write for(j = 0; j < avctx->width; j += 2){unpack byte}
> >>> without an additional condition
> >> Wouldn't this write one sample too much ?
> > 
> > It will, but since stride >= width and stride is even, this is not an
> > out of bounds write.
> 
> Humm, isn't align 1 for PAL8 according to avcodec_align_dimensions ?

Indeed (unless it's SMC codec).

> > Alternatively you can introduce PIX_FMT_PAL4 and do not unpack source at
> > all ;).
> 
> Well, maybe it is not worth, since this would require conversions from
> and to it.

We would benefit from conversion to PAL8 at least though.
 
> -- 
> Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
> Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
> FFmpeg maintainer                                  http://www.ffmpeg.org

> Index: libavcodec/msrle.c
> ===================================================================
> --- libavcodec/msrle.c	(revision 18964)
> +++ libavcodec/msrle.c	(working copy)
> @@ -55,7 +55,19 @@
>  
>      s->avctx = avctx;
>  
> -    avctx->pix_fmt = PIX_FMT_PAL8;
> +    switch (avctx->bits_per_coded_sample) {
> +    case 4:
> +    case 8:
> +        avctx->pix_fmt = PIX_FMT_PAL8;
> +        break;
> +    case 24:
> +        avctx->pix_fmt = PIX_FMT_BGR24;
> +        break;
> +    default:
> +        av_log(avctx, AV_LOG_ERROR, "unsupported bits per sample\n");
> +        return -1;
> +    }
> +
>      s->frame.data[0] = NULL;
>  
>      return 0;
> @@ -68,6 +80,7 @@
>      const uint8_t *buf = avpkt->data;
>      int buf_size = avpkt->size;
>      MsrleContext *s = avctx->priv_data;
> +    int istride = FFALIGN(avctx->width*avctx->bits_per_coded_sample, 32) / 8;
>  
>      s->buf = buf;
>      s->size = buf_size;
> @@ -88,8 +101,31 @@
>          }
>      }
>  
> -    ff_msrle_decode(avctx, (AVPicture*)&s->frame, avctx->bits_per_coded_sample, buf, buf_size);
> +    /* FIXME how to correctly detected RLE ??? */

s/detected/detect/

> +    if (avctx->height * istride == avpkt->size) { /* assume uncompressed */
> +        int linesize = avctx->width * avctx->bits_per_coded_sample / 8;
> +        uint8_t *ptr = s->frame.data[0];
> +        uint8_t *buf = avpkt->data + (avctx->height-1)*istride;
> +        int i, j;
>  
> +        for (i = 0; i < avctx->height; i++) {
> +            if (avctx->bits_per_coded_sample == 4) {
> +                for (j = 0; j < linesize; j++) {
> +                    ptr[j*2+0] = buf[j] >> 4;
> +                    ptr[j*2+1] = buf[j] & 0xf;
> +                }
> +                if (j*2 < avctx->width)
> +                    ptr[j*2+0] = buf[j] >> 4;

again, maybe
 for (j = 0; j < width; j += 2) {
  ptr[j+0] = buf[j>>1] >> 4;
  ptr[j+1] = buf[j>>1] & 0xF;
 }
 if (width & 1) ptr[width-1] = buf[j>>1] >> 4;

thus you'll get rid of unneeded multiplication as well
but if you think this is better, commit this

> +            } else {
> +                memcpy(ptr, buf, linesize);
> +            }
> +            buf -= istride;
> +            ptr += s->frame.linesize[0];
> +        }
> +    } else {
> +        ff_msrle_decode(avctx, (AVPicture*)&s->frame, avctx->bits_per_coded_sample, buf, buf_size);
> +    }
> +
>      *data_size = sizeof(AVFrame);
>      *(AVFrame*)data = s->frame;