[FFmpeg-devel] [RFC/PATCH]Support signed j2k images via libopenjpeg

[Michael Bradshaw]

> diff --git a/libavcodec/libopenjpegdec.c b/libavcodec/libopenjpegdec.c
> index 0543e3a..d16c2df 100644
> --- a/libavcodec/libopenjpegdec.c
> +++ b/libavcodec/libopenjpegdec.c
> @@ -162,7 +162,7 @@ static inline void
libopenjpeg_copy_to_packed8(AVFrame *picture, opj_image_t *im
>          img_ptr = picture->data[0] + y*picture->linesize[0];
>          for (x = 0; x < picture->width; x++, index++) {
>              for (c = 0; c < image->numcomps; c++) {
> -                *img_ptr++ = image->comps[c].data[index];
> +                *img_ptr++ = 0x80 * image->comps[c].sgnd +
image->comps[c].data[index];
>              }
>          }
>      }
> @@ -180,7 +180,7 @@ static inline void
libopenjpeg_copy_to_packed16(AVFrame *picture, opj_image_t *i
>          img_ptr = (uint16_t*) (picture->data[0] +
y*picture->linesize[0]);
>          for (x = 0; x < picture->width; x++, index++) {
>              for (c = 0; c < image->numcomps; c++) {
> -                *img_ptr++ = image->comps[c].data[index] << adjust[c];
> +                *img_ptr++ = 0x8000 * image->comps[c].sgnd +
(image->comps[c].data[index] << adjust[c]);

This might be something to fix in a different patch, separate from this
one, but while we're talking about signed-ness we might as well talk about
image->comps[c].data[index] << adjust[c], which is technically UB for
negative values. I propose casting image->comps[c].data[index] to unsigned
int, unless anyone has a better suggestion.

>              }
>          }
>      }
> @@ -196,7 +196,7 @@ static inline void libopenjpeg_copyto8(AVFrame
*picture, opj_image_t *image) {
>          for (y = 0; y < image->comps[index].h; y++) {
>              img_ptr = picture->data[index] + y *
picture->linesize[index];
>              for (x = 0; x < image->comps[index].w; x++) {
> -                *img_ptr = (uint8_t) *comp_data;
> +                *img_ptr = 0x80 * image->comps[index].sgnd + (uint8_t)
*comp_data;

I'd personally get rid of this (uint8_t) cast, while you're at it. I know
it's been there for a couple years but it's not really accomplishing much
and I think the type promotion/conversion rules in the arithmetic are more
obvious with it gone.

>                  img_ptr++;
>                  comp_data++;
>              }
> @@ -217,7 +217,7 @@ static inline void libopenjpeg_copyto16(AVFrame
*picture, opj_image_t *image) {
>          for (y = 0; y < image->comps[index].h; y++) {
>              img_ptr = (uint16_t*) (picture->data[index] + y *
picture->linesize[index]);
>              for (x = 0; x < image->comps[index].w; x++) {
> -                *img_ptr = *comp_data << adjust[index];
> +                *img_ptr = 0x8000 * image->comps[index].sgnd +
(*comp_data << adjust[index]);

Same thing here as previously talked about: cast *comp_data to unsigned int
first?

>                  img_ptr++;
>                  comp_data++;
>              }

Otherwise, LGTM (though should be a format-patch rather than a diff, though
I guess this is a pseudo RFC). Fixes decoding the sample in #3283 and
doesn't seem to have broken anything new.

--Michael Bradshaw