fix(ffmpeg): 10 bit video has wrong green channel (#4935)

Due to apparent limitations or bugs in swscale, there is a problem
reading the 16 bit green channel that is not 4 byte aligned like red and
blue. Reading each channel into its own plane solves the issue.

This also addresses an existing comment about 32 bit float being read as
16 bit. This needed the same mechanism of reading separate planes, so
the same code is reused to implement that.

Resolves #4901

I added a 10 bit test file encoded with VP9 and Matroska, same as the
existing 8 bit test file to maximize compatibility. I verified this now
converts correctly to a HDR PNG file.

Signed-off-by: Brecht Van Lommel <[email protected]>

Author: brechtvl

src/ffmpeg.imageio/ffmpeginput.cpp

@@ -398,8 +398,10 @@ FFmpegInput::open(const std::string& name, ImageSpec& spec)
     case AV_PIX_FMT_GBRAP12LE:
     case AV_PIX_FMT_P016LE:
     case AV_PIX_FMT_P016BE:
-        datatype         = TypeUInt16;
-        m_dst_pix_format = AV_PIX_FMT_RGB48;
+        datatype = TypeUInt16;
+        // Must use planar format because swscale does not handle
+        // interleaved correctly for 16 bit.
+        m_dst_pix_format = AV_PIX_FMT_GBRP16;
         break;
     // Grayscale 8 bit
     case AV_PIX_FMT_GRAY8:
@@ -456,29 +458,27 @@ FFmpegInput::open(const std::string& name, ImageSpec& spec)
     case AV_PIX_FMT_YUVA444P16BE:
     case AV_PIX_FMT_YUVA444P16LE:
     case AV_PIX_FMT_GBRAP16:
-        nchannels        = 4;
-        datatype         = TypeUInt16;
-        m_dst_pix_format = AV_PIX_FMT_RGBA64;
+        nchannels = 4;
+        datatype  = TypeUInt16;
+        // Must use planar format because swscale does not handle
+        // interleaved correctly for 16 bit.
+        m_dst_pix_format = AV_PIX_FMT_GBRAP16;
         break;
     // RGB float
     case AV_PIX_FMT_GBRPF32BE:
     case AV_PIX_FMT_GBRPF32LE:
-        nchannels        = 3;
-        datatype         = TypeFloat;
-        m_dst_pix_format = AV_PIX_FMT_RGB48;  // ? AV_PIX_FMT_GBRPF32
-        // FIXME: They don't have a type for RGB float, only GBR float.
-        // Yuck. Punt for now and save as uint16 RGB. If people care, we
-        // can return and ask for GBR float and swap order.
+        nchannels = 3;
+        datatype  = TypeFloat;
+        // Must use planar format as there is no interleaved 32 bit float.
+        m_dst_pix_format = AV_PIX_FMT_GBRPF32;
         break;
     // RGBA float
     case AV_PIX_FMT_GBRAPF32BE:
     case AV_PIX_FMT_GBRAPF32LE:
-        nchannels        = 4;
-        datatype         = TypeFloat;
-        m_dst_pix_format = AV_PIX_FMT_RGBA64;  // ? AV_PIX_FMT_GBRAPF32
-        // FIXME: They don't have a type for RGBA float, only GBRA float.
-        // Yuck. Punt for now and save as uint16 RGB. If people care, we
-        // can return and ask for GBRA float and swap order.
+        nchannels = 4;
+        datatype  = TypeFloat;
+        // Must use planar format as there is no interleaved 32 bit float.
+        m_dst_pix_format = AV_PIX_FMT_GBRAPF32;
         break;
 
     // Everything else is regular 8 bit RGB
@@ -571,7 +571,31 @@ FFmpegInput::seek_subimage(int subimage, int miplevel)
     return true;
 }
 
+template<typename T, int nchannels>
+static bool
+read_planar_scanline(void* data, int y, int width, AVFrame* rgb_frame)
+{
+    static_assert(nchannels == 3 || nchannels == 4);
+
+    const T* in_planes[nchannels];
+    const int swizzle[4] = { 1, 2, 0, 3 };  // GBR to RGB
+    for (int channel = 0; channel < nchannels; ++channel) {
+        if (rgb_frame->data[channel] == nullptr) {
+            return false;
+        }
+        in_planes[swizzle[channel]] = reinterpret_cast<const T*>(
+            rgb_frame->data[channel] + y * rgb_frame->linesize[channel]);
+    }
 
+    T* out = static_cast<T*>(data);
+    for (int x = 0; x < width; ++x) {
+        for (int channel = 0; channel < nchannels; ++channel) {
+            out[channel] = in_planes[channel][x];
+        }
+        out += nchannels;
+    }
+    return true;
+}
 
 bool
 FFmpegInput::read_native_scanline(int subimage, int miplevel, int y, int /*z*/,
@@ -583,14 +607,39 @@ FFmpegInput::read_native_scanline(int subimage, int miplevel, int y, int /*z*/,
     if (!m_read_frame) {
         read_frame(m_subimage);
     }
-    if (m_rgb_frame->data[0]) {
-        memcpy(data, m_rgb_frame->data[0] + y * m_rgb_frame->linesize[0],
-               m_stride);
-        return true;
-    } else {
-        errorfmt("Error reading frame");
-        return false;
+    if (m_spec.format == TypeUInt8 || m_spec.nchannels == 1) {
+        if (m_rgb_frame->data[0]) {
+            memcpy(data, m_rgb_frame->data[0] + y * m_rgb_frame->linesize[0],
+                   m_stride);
+            return true;
+        }
+    } else if (m_spec.format == TypeUInt16) {
+        if (m_spec.nchannels == 3) {
+            if (read_planar_scanline<uint16_t, 3>(data, y, m_spec.width,
+                                                  m_rgb_frame)) {
+                return true;
+            }
+        } else if (m_spec.nchannels == 4) {
+            if (read_planar_scanline<uint16_t, 4>(data, y, m_spec.width,
+                                                  m_rgb_frame)) {
+                return true;
+            }
+        }
+    } else if (m_spec.format == TypeFloat) {
+        if (m_spec.nchannels == 3) {
+            if (read_planar_scanline<float, 3>(data, y, m_spec.width,
+                                               m_rgb_frame)) {
+                return true;
+            }
+        } else if (m_spec.nchannels == 4) {
+            if (read_planar_scanline<float, 4>(data, y, m_spec.width,
+                                               m_rgb_frame)) {
+                return true;
+            }
+        }
     }
+    errorfmt("Error reading frame");
+    return false;
 }
 
 

testsuite/ffmpeg/ref/out-ffmpeg6.1.txt

@@ -85,3 +85,26 @@ ref/vp9_display_p3.mkv :  192 x  108, 3 channel, uint8 FFmpeg movie
     oiio:BitsPerSample: 8
     oiio:Movie: 1
     oiio:subimages: 7
+Reading ref/vp9_rec2100_pq.mkv
+ref/vp9_rec2100_pq.mkv :  384 x  216, 3 channel, uint10 FFmpeg movie
+    2 subimages: 384x216 [u10,u10,u10]
+ subimage  0:  384 x  216, 3 channel, uint10 FFmpeg movie
+    SHA-1: 87BB7BBE57131F08F0336964256F2C190FE26A69
+    channel list: R, G, B
+    CICP: 9, 16, 9, 1
+    ENCODER: "Lavf62.3.100"
+    FramesPerSecond: 24/1 (24)
+    ffmpeg:codec_name: "Google VP9"
+    oiio:BitsPerSample: 10
+    oiio:Movie: 1
+    oiio:subimages: 2
+ subimage  1:  384 x  216, 3 channel, uint10 FFmpeg movie
+    SHA-1: 3820BE456CC3F4ADD3425E0C9397C83678A5BA8A
+    channel list: R, G, B
+    CICP: 9, 16, 9, 1
+    ENCODER: "Lavf62.3.100"
+    FramesPerSecond: 24/1 (24)
+    ffmpeg:codec_name: "Google VP9"
+    oiio:BitsPerSample: 10
+    oiio:Movie: 1
+    oiio:subimages: 2

testsuite/ffmpeg/ref/out-ffmpeg8.0.txt

@@ -85,3 +85,26 @@ ref/vp9_display_p3.mkv :  192 x  108, 3 channel, uint8 FFmpeg movie
     oiio:BitsPerSample: 8
     oiio:Movie: 1
     oiio:subimages: 7
+Reading ref/vp9_rec2100_pq.mkv
+ref/vp9_rec2100_pq.mkv :  384 x  216, 3 channel, uint10 FFmpeg movie
+    2 subimages: 384x216 [u10,u10,u10]
+ subimage  0:  384 x  216, 3 channel, uint10 FFmpeg movie
+    SHA-1: 87BB7BBE57131F08F0336964256F2C190FE26A69
+    channel list: R, G, B
+    CICP: 9, 16, 9, 1
+    ENCODER: "Lavf62.3.100"
+    FramesPerSecond: 24/1 (24)
+    ffmpeg:codec_name: "Google VP9"
+    oiio:BitsPerSample: 10
+    oiio:Movie: 1
+    oiio:subimages: 2
+ subimage  1:  384 x  216, 3 channel, uint10 FFmpeg movie
+    SHA-1: 3820BE456CC3F4ADD3425E0C9397C83678A5BA8A
+    channel list: R, G, B
+    CICP: 9, 16, 9, 1
+    ENCODER: "Lavf62.3.100"
+    FramesPerSecond: 24/1 (24)
+    ffmpeg:codec_name: "Google VP9"
+    oiio:BitsPerSample: 10
+    oiio:Movie: 1
+    oiio:subimages: 2

testsuite/ffmpeg/ref/vp9_rec2100_pq.mkv

@@ -5,6 +5,6 @@
 # https://github.com/AcademySoftwareFoundation/OpenImageIO
 
 imagedir = "ref/"
-files = [ "vp9_display_p3.mkv" ]
+files = [ "vp9_display_p3.mkv", "vp9_rec2100_pq.mkv" ]
 for f in files:
     command = command + info_command (os.path.join(imagedir, f))