[FFmpeg-devel] [PATCH 2/2] avcodec/dpx: improve decoding support for 10 bit depth

[Carl Eugen Hoyos]

2018-12-06 11:56 GMT+01:00, Paul B Mahol <onemda at gmail.com>:
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> ---
>  libavcodec/dpx.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c
> index 0297287938..84894abda5 100644
> --- a/libavcodec/dpx.c
> +++ b/libavcodec/dpx.c
> @@ -50,6 +50,24 @@ static unsigned int read32(const uint8_t **ptr, int
> is_big)
>      return temp;
>  }
>
> +static uint16_t read10in32_gray(const uint8_t **ptr, uint32_t *lbuf,
> +                                int *n_datum, int is_big, int shift)
> +{
> +    uint16_t temp;
> +
> +    if (*n_datum)
> +        (*n_datum)--;
> +    else {
> +        *lbuf = read32(ptr, is_big);
> +        *n_datum = 2;
> +    }
> +

> +    temp = *lbuf >> shift & 0x3FF;
> +    *lbuf = *lbuf >> 10;
> +
> +    return temp;

Isn't my code simpler?
One variable less, same number of operations iirc.


> +}
> +
>  static uint16_t read10in32(const uint8_t **ptr, uint32_t * lbuf,
>                                    int * n_datum, int is_big, int shift)
>  {
> @@ -107,6 +125,7 @@ static int decode_frame(AVCodecContext *avctx,
>      AVFrame *const p = data;
>      uint8_t *ptr[AV_NUM_DATA_POINTERS];
>      uint32_t header_version, version = 0;
> +    const uint8_t *creator;
>
>      unsigned int offset;
>      int magic_num, endian;
> @@ -151,6 +170,9 @@ static int decode_frame(AVCodecContext *avctx,
>          av_log(avctx, AV_LOG_WARNING, "Unknown header format version
> %s.\n",
>                 av_fourcc2str(header_version));
>

> +    buf = avpkt->data + 160;
> +    creator = buf;

I wonder why you split the version but not the creator
and why the metadata isn't set.

> +
>      // Check encryption
>      buf = avpkt->data + 660;
>      ret = read32(&buf, endian);
> @@ -320,6 +342,7 @@ static int decode_frame(AVCodecContext *avctx,
>      case 51121:
>          avctx->pix_fmt = AV_PIX_FMT_GBRAP12;

>          break;
> +    case 6100:

Looks unrelated.

>      case 6101:
>          avctx->pix_fmt = AV_PIX_FMT_GRAY10;
>          break;
> @@ -373,13 +396,17 @@ static int decode_frame(AVCodecContext *avctx,
>                                  (uint16_t*)ptr[1],
>                                  (uint16_t*)ptr[2],
>                                  (uint16_t*)ptr[3]};
> -            int shift = packing == 1 ? 22 : 20;
> +            int shift = elements > 1 ? packing == 1 ? 22 : 20 : packing ==
> 1 ? 2 : 0;

Wouldn't it be simpler to add "20" in read10in32()?

>              for (y = 0; y < avctx->width; y++) {
>                  if (elements >= 3)
>                      *dst[2]++ = read10in32(&buf, &rgbBuffer,
>                                             &n_datum, endian, shift);
> -                *dst[0]++ = read10in32(&buf, &rgbBuffer,
> -                                       &n_datum, endian, shift);
> +                if (elements == 1)
> +                    *dst[0]++ = read10in32_gray(&buf, &rgbBuffer,
> +                                                &n_datum, endian, shift);
> +                else
> +                    *dst[0]++ = read10in32(&buf, &rgbBuffer,
> +                                           &n_datum, endian, shift);
>                  if (elements >= 2)
>                      *dst[1]++ = read10in32(&buf, &rgbBuffer,
>                                             &n_datum, endian, shift);
> @@ -388,7 +415,8 @@ static int decode_frame(AVCodecContext *avctx,
>                      read10in32(&buf, &rgbBuffer,
>                                 &n_datum, endian, shift);
>              }

> -            n_datum = 0;
> +            if (version != 2 || !memcmp(creator, "GraphicsMagick", 14))

This looks wrong to me:
Where in the specification does it say that this depends on the version?

> +                n_datum = 0;
>              for (i = 0; i < elements; i++)
>                  ptr[i] += p->linesize[i];
>          }

Thank you, Carl Eugen