[FFmpeg-devel] [PATCH 2/2] acvodec/lipopenjpeg: Improve XYZ color space detection

[Michael Bradshaw]

On Tue, Mar 3, 2015 at 1:19 AM, Vilius Grigaliūnas <
vilius.grigaliunas at gmail.com> wrote:

> Input files in XYZ color space are incorrecly detected as RGB which results
> in incorrect output colors.
>
> This fixes pixel format detection order (in increasing bit depth to
> match libopenjpeg_matches_pix_fmt) when color space provided by
> libopenjepg is unknown.
> ---
>  libavcodec/libopenjpegdec.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/libopenjpegdec.c b/libavcodec/libopenjpegdec.c
> index 1cd1b9b..489040e 100644
> --- a/libavcodec/libopenjpegdec.c
> +++ b/libavcodec/libopenjpegdec.c
> @@ -77,7 +77,7 @@ static const enum AVPixelFormat
> libopenjpeg_yuv_pix_fmts[]  = {
>      YUV_PIXEL_FORMATS
>  };
>  static const enum AVPixelFormat libopenjpeg_all_pix_fmts[]  = {
> -    RGB_PIXEL_FORMATS, GRAY_PIXEL_FORMATS, YUV_PIXEL_FORMATS,
> XYZ_PIXEL_FORMATS
> +    AV_PIX_FMT_RGB24, AV_PIX_FMT_RGBA, XYZ_PIXEL_FORMATS,
> AV_PIX_FMT_RGB48, AV_PIX_FMT_RGBA64, GRAY_PIXEL_FORMATS, YUV_PIXEL_FORMATS
>

That seems like the least intrusive fix, and the logic looks right to me. I
don't know if 8-bit per component XYZ will ever be supported in ffmpeg, but
if so then deciding between rgb24 and xyz8 will need a different method.
The mixing of AV_PIX_FMT_* enums and *_PIXEL_FORMATS macros looks
tedious/fragile.

I'd say the above patch is okay. Alternatively, something like the
following could be done, I think:

diff --git a/libavcodec/libopenjpegdec.c b/libavcodec/libopenjpegdec.c
index 02b1ceb..da53e05 100644
--- a/libavcodec/libopenjpegdec.c
+++ b/libavcodec/libopenjpegdec.c
@@ -76,7 +76,23 @@ static const enum AVPixelFormat
libopenjpeg_gray_pix_fmts[] = {
 static const enum AVPixelFormat libopenjpeg_yuv_pix_fmts[]  = {
     YUV_PIXEL_FORMATS
 };
-static const enum AVPixelFormat libopenjpeg_all_pix_fmts[]  = {
+// OpenJPEG currently doesn't support detecting xyz. If the file is
+// a JP2 file, then it should have the color space saved, and
+// OpenJPEG should be able to detect what it is. In the event that
+// the file is a JP2 and OpenJPEG can't detect what the color space
+// is, then we should try guessing xyz first, as that's the only
+// likely candidate (since OpenJPEG can detect gray/yuv/rgb, we can
+// effectively rule those out). If the file is a J2K codestream,
+// then the color format isn't known and must be guessed. In that
+// event, rgb should be guessed first, as it tends to be more
+// common. Hence, we have two arrays that prioritize search order
+// for pixel formats, based on the image type: for JP2 files,
+// libopenjpeg_jp2_all_pix_fmts; and for J2K files,
+// libopenjpeg_j2k_all_pix_fmts.
+static const enum AVPixelFormat libopenjpeg_jp2_all_pix_fmts[]  = {
+    XYZ_PIXEL_FORMATS, GRAY_PIXEL_FORMATS, YUV_PIXEL_FORMATS,
RGB_PIXEL_FORMATS
+};
+static const enum AVPixelFormat libopenjpeg_j2k_all_pix_fmts[]  = {
     RGB_PIXEL_FORMATS, GRAY_PIXEL_FORMATS, YUV_PIXEL_FORMATS,
XYZ_PIXEL_FORMATS
 };

@@ -123,7 +139,7 @@ static inline int libopenjpeg_matches_pix_fmt(const
opj_image_t *image, enum AVP
     return match;
 }

-static inline enum AVPixelFormat libopenjpeg_guess_pix_fmt(const
opj_image_t *image) {
+static inline enum AVPixelFormat libopenjpeg_guess_pix_fmt(const
opj_image_t *image, int is_jp2) {
     int index;
     const enum AVPixelFormat *possible_fmts = NULL;
     int possible_fmts_nb = 0;
@@ -142,8 +158,13 @@ static inline enum AVPixelFormat
libopenjpeg_guess_pix_fmt(const opj_image_t *im
         possible_fmts_nb = FF_ARRAY_ELEMS(libopenjpeg_yuv_pix_fmts);
         break;
     default:
-        possible_fmts    = libopenjpeg_all_pix_fmts;
-        possible_fmts_nb = FF_ARRAY_ELEMS(libopenjpeg_all_pix_fmts);
+        if (is_jp2) {
+            possible_fmts    = libopenjpeg_jp2_all_pix_fmts;
+            possible_fmts_nb =
FF_ARRAY_ELEMS(libopenjpeg_jp2_all_pix_fmts);
+        } else {
+            possible_fmts    = libopenjpeg_j2k_all_pix_fmts;
+            possible_fmts_nb =
FF_ARRAY_ELEMS(libopenjpeg_j2k_all_pix_fmts);
+        }
         break;
     }

@@ -263,6 +284,7 @@ static int libopenjpeg_decode_frame(AVCodecContext
*avctx,
     int width, height, ret;
     int pixel_size = 0;
     int ispacked   = 0;
+    int is_jp2;
     int i;

     *got_frame = 0;
@@ -272,12 +294,14 @@ static int libopenjpeg_decode_frame(AVCodecContext
*avctx,
         (AV_RB32(buf + 4) == JP2_SIG_TYPE) &&
         (AV_RB32(buf + 8) == JP2_SIG_VALUE)) {
         dec = opj_create_decompress(CODEC_JP2);
+        is_jp2 = 1;
     } else {
         /* If the AVPacket contains a jp2c box, then skip to
          * the starting byte of the codestream. */
         if (AV_RB32(buf + 4) == AV_RB32("jp2c"))
             buf += 8;
         dec = opj_create_decompress(CODEC_J2K);
+        is_jp2 = 0;
     }

     if (!dec) {
@@ -320,7 +344,7 @@ static int libopenjpeg_decode_frame(AVCodecContext
*avctx,
             avctx->pix_fmt = AV_PIX_FMT_NONE;

     if (avctx->pix_fmt == AV_PIX_FMT_NONE)
-        avctx->pix_fmt = libopenjpeg_guess_pix_fmt(image);
+        avctx->pix_fmt = libopenjpeg_guess_pix_fmt(image, is_jp2);

     if (avctx->pix_fmt == AV_PIX_FMT_NONE) {
         av_log(avctx, AV_LOG_ERROR, "Unable to determine pixel format\n");