After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 780100 - video: add support for planar GBR and GBRA formats with 12 bits per channel, GBRA with 8, 10 and 12 bits per channel, 4:2:0/4:2:2/4:4:4 YUV with 12 bits per channel
video: add support for planar GBR and GBRA formats with 12 bits per channel, ...
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.11.x
Other All
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-15 16:40 UTC by emeric.grange
Modified: 2018-11-03 11:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add GBR_12LE/BE and GBRA_12LE/BE pixel format mapping for CineForm decoder (1.08 KB, patch)
2017-03-15 16:40 UTC, emeric.grange
committed Details | Review
Add GBR_12LE/BE and GBRA_12LE/BE pixel format (13.32 KB, patch)
2017-03-15 16:40 UTC, emeric.grange
none Details | Review
Add ARGB64_LE/BE pixel format (4.79 KB, patch)
2017-03-15 16:58 UTC, emeric.grange
needs-work Details | Review
Add GBRA_10LE/BE, GBR_12LE/BE, GBRA_12LE/BE pixel formats (17.83 KB, patch)
2017-03-16 10:48 UTC, emeric.grange
needs-work Details | Review
video: Add GBRA_10LE/BE, GBR_12LE/BE, GBRA_12LE/BE pixel formats (18.66 KB, patch)
2017-03-16 14:07 UTC, Sebastian Dröge (slomo)
none Details | Review
cineform playback error log (9.88 KB, text/plain)
2017-03-16 14:14 UTC, emeric.grange
  Details
video: Add GBRA_10LE/BE, GBR_12LE/BE, GBRA_12LE/BE pixel formats (18.94 KB, patch)
2017-03-16 14:40 UTC, Sebastian Dröge (slomo)
none Details | Review
video: Add GBRA_10LE/BE, GBR_12LE/BE, GBRA_12LE/BE pixel formats (19.22 KB, patch)
2017-03-16 14:43 UTC, Sebastian Dröge (slomo)
committed Details | Review
video: Add I420/I422/Y444_10LE/BE and GBRA video formats (22.03 KB, patch)
2017-03-16 15:23 UTC, Sebastian Dröge (slomo)
none Details | Review
video: Add I420/I422/Y444_12LE/BE and GBRA video formats (22.03 KB, patch)
2017-03-16 15:23 UTC, Sebastian Dröge (slomo)
none Details | Review
avcodecmap: Add mappings for I420/I422/Y444_12LE/BE and GBRA (1.55 KB, patch)
2017-03-16 15:23 UTC, Sebastian Dröge (slomo)
committed Details | Review
video: Add I420/I422/Y444_12LE/BE and GBRA video formats (22.03 KB, patch)
2017-03-16 15:38 UTC, Sebastian Dröge (slomo)
committed Details | Review
video-format: Add Since markers to all new formats from 1.2 (5.64 KB, patch)
2017-03-16 15:38 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description emeric.grange 2017-03-15 16:40:01 UTC
Created attachment 348013 [details] [review]
Add GBR_12LE/BE and GBRA_12LE/BE pixel format mapping for CineForm decoder

Add support for BGR and BGRA with 12 bits per channel so we can use the CineForm decoder from ffmpeg.
This is a work in progress!
Comment 1 emeric.grange 2017-03-15 16:40:36 UTC
Created attachment 348014 [details] [review]
Add GBR_12LE/BE and GBRA_12LE/BE pixel format
Comment 2 Sebastian Dröge (slomo) 2017-03-15 16:44:20 UTC
You say it's WIP. Do you need any help with any parts of it? What parts are missing?

From a (very fast) look over the patch, it looks like it does all the right things but I'll take a closer look later. While we're at it, we might also want to add GBRA_10LE/BE formats though, for consistency :)
Comment 3 emeric.grange 2017-03-15 16:58:07 UTC
Created attachment 348015 [details] [review]
Add ARGB64_LE/BE pixel format

I also have this patch to add ARGB64_LE and ARGB64_BE (or b64a).
I was not able to test those patches today because my GST version seems broken, but at least they build.

Like the GBR12 patch, it's mostly to know if I'm on the right path. I'll be available tomorrow on IRC to talk about this. Thanks.
Comment 4 emeric.grange 2017-03-15 17:08:41 UTC
(In reply to Sebastian Dröge (slomo) from comment #2)
> You say it's WIP. Do you need any help with any parts of it? What parts are
> missing?
> 
> From a (very fast) look over the patch, it looks like it does all the right
> things but I'll take a closer look later. While we're at it, we might also
> want to add GBRA_10LE/BE formats though, for consistency :)

I was only able to build the patch on today's git, not test it, that's why the WIP. I think I have an unrelated problem with gst-libav (all my attempts are ending up with "libav :0:: get_buffer() failed" even when not trying CineForm videos.
If I use my own CineForm plugin (or any other plugins from good) I end up stuck on "Redistribute latency...".

Regarding the first patch, it's mostly copy/past but I'm not sure about the GST_VIDEO_PACK_FLAG_TRUNCATE_RANGE section. I'll add GBRA_10LE/BE tomorrow.
For the second patch, I'm not sure about the "swap" approach alltogether.
Comment 5 emeric.grange 2017-03-16 10:48:43 UTC
Created attachment 348073 [details] [review]
Add GBRA_10LE/BE, GBR_12LE/BE, GBRA_12LE/BE pixel formats

This patch adds GBRA_10LE/BE and has a small fix.
I was able to backport everything to 1.10.4 to test, but while I can indeed play CineForm YUV422 I still cannot play RGB/A videos, so something may still be missing.

ERROR: from element /GstPipeline:pipeline0/GstDecodeBin:decodebin0/avdec_cfhd:avdec_cfhd0: Unable to allocate memory
Additional debug info:
gstavviddec.c(857): gst_ffmpegviddec_get_buffer2 (): /GstPipeline:pipeline0/GstDecodeBin:decodebin0/avdec_cfhd:avdec_cfhd0:
The downstream pool failed to allocated buffer.
ERROR: pipeline doesn't want to preroll.
Comment 6 Sebastian Dröge (slomo) 2017-03-16 11:14:51 UTC
Can you attach a debug log? The patch looks complete to me. Should probably also add equivalents of the AV_PIX_FMT_YUV4XXP12LE/BE formats from ffmpeg, but not relevant here. I'll just do that later :)

Did you recompile videoconvert, etc after your patch?
Comment 7 Sebastian Dröge (slomo) 2017-03-16 14:07:51 UTC
Created attachment 348089 [details] [review]
video: Add GBRA_10LE/BE, GBR_12LE/BE, GBRA_12LE/BE pixel formats
Comment 8 Sebastian Dröge (slomo) 2017-03-16 14:08:53 UTC
Comment on attachment 348073 [details] [review]
Add GBRA_10LE/BE, GBR_12LE/BE, GBRA_12LE/BE pixel formats

This did not compile cleanly, I fixed that now. Please base any future work on this patch on the one I just attached with those fixes :)
Comment 9 Nicolas Dufresne (ndufresne) 2017-03-16 14:13:32 UTC
Review of attachment 348073 [details] [review]:

::: gst-libs/gst/video/video-format.h
@@ +103,3 @@
+ * @GST_VIDEO_FORMAT_GBR_12LE: planar 4:4:4 RGB, 12 bits per channel
+ * @GST_VIDEO_FORMAT_GBRA_12BE: planar 4:4:4:4 ARGB, 12 bits per channel
+ * @GST_VIDEO_FORMAT_GBRA_12LE: planar 4:4:4:4 ARGB, 12 bits per channel

I didn't realize this was planar RGB. I'm thinking out loud here, but could we create a notation to differentiate packed RGB from planar RGB. When I read the format first, I though it was going to be packed, with 64bit pixels, and each component being padded. Clearly this is not the case.

Other then that, I'm surprised of the component / plane order, but I need to document myself.

::: gst-libs/gst/video/video-info.c
@@ +918,3 @@
+    case GST_VIDEO_FORMAT_GBRA_12LE:
+    case GST_VIDEO_FORMAT_GBRA_12BE:
+      info->stride[0] = GST_ROUND_UP_4 (width * 2);

Your pack/unpack function only require 16bit alignment. Do you really need to round up here ?
Comment 10 emeric.grange 2017-03-16 14:14:37 UTC
Created attachment 348090 [details]
cineform playback error log

Here is the cineform playback error log
Comment 11 Sebastian Dröge (slomo) 2017-03-16 14:16:45 UTC
(In reply to Nicolas Dufresne (stormer) from comment #9)

> ::: gst-libs/gst/video/video-format.h
> @@ +103,3 @@
> + * @GST_VIDEO_FORMAT_GBR_12LE: planar 4:4:4 RGB, 12 bits per channel
> + * @GST_VIDEO_FORMAT_GBRA_12BE: planar 4:4:4:4 ARGB, 12 bits per channel
> + * @GST_VIDEO_FORMAT_GBRA_12LE: planar 4:4:4:4 ARGB, 12 bits per channel
> 
> I didn't realize this was planar RGB. I'm thinking out loud here, but could
> we create a notation to differentiate packed RGB from planar RGB. When I
> read the format first, I though it was going to be packed, with 64bit
> pixels, and each component being padded. Clearly this is not the case.
> 
> Other then that, I'm surprised of the component / plane order, but I need to
> document myself.

We already support GBR in various versions, nothing new here :)
Comment 12 Nicolas Dufresne (ndufresne) 2017-03-16 14:19:54 UTC
What's new is that I notice that our naming does not help differentiating planar and packed/interleaved RGB. What happens now if someone wants to add an interleave variant of these ?
Comment 13 Sebastian Dröge (slomo) 2017-03-16 14:28:53 UTC
(In reply to emeric.grange from comment #10)
> Created attachment 348090 [details]
> cineform playback error log
> 
> Here is the cineform playback error log

I couldn't find the problem while shortly looking, but there's definitely something wrong with the patch. It also crashes in various ways with very simple pipelines:

gst-launch-1.0 videotestsrc ! "video/x-raw,format=GBR_12BE" ! videoconvert ! "video/x-raw,format=RGBA" ! glimagesink
gst-launch-1.0 videotestsrc ! "video/x-raw,format=GBRA_10BE" ! videoconvert ! "video/x-raw,format=RGBA" ! glimagesink

Same with the LE variants. GBR_10BE/LE works fine though (the old one). I'd look into these problems first, that might also solve the gst-libav ones.
Comment 14 Nicolas Dufresne (ndufresne) 2017-03-16 14:40:21 UTC
Time to use valgrind, and to review the offsets.
Comment 15 Sebastian Dröge (slomo) 2017-03-16 14:40:35 UTC
Created attachment 348092 [details] [review]
video: Add GBRA_10LE/BE, GBR_12LE/BE, GBRA_12LE/BE pixel formats
Comment 16 Sebastian Dröge (slomo) 2017-03-16 14:41:48 UTC
With this updated the GBR variants work, and also the GBR12 CFHD video. GBRA gets colors wrong.
Comment 17 Sebastian Dröge (slomo) 2017-03-16 14:43:29 UTC
Created attachment 348093 [details] [review]
video: Add GBRA_10LE/BE, GBR_12LE/BE, GBRA_12LE/BE pixel formats
Comment 18 Sebastian Dröge (slomo) 2017-03-16 14:44:07 UTC
And with this also the GBRA variants and the corresponding CFHD video are working.
Comment 19 Sebastian Dröge (slomo) 2017-03-16 15:23:04 UTC
Created attachment 348097 [details] [review]
video: Add I420/I422/Y444_10LE/BE and GBRA video formats
Comment 20 Sebastian Dröge (slomo) 2017-03-16 15:23:21 UTC
Created attachment 348098 [details] [review]
video: Add I420/I422/Y444_12LE/BE and GBRA video formats
Comment 21 Sebastian Dröge (slomo) 2017-03-16 15:23:28 UTC
Created attachment 348099 [details] [review]
avcodecmap: Add mappings for I420/I422/Y444_12LE/BE and GBRA
Comment 22 Sebastian Dröge (slomo) 2017-03-16 15:24:04 UTC
And for completeness a few other missing formats. GBRA is broken currently
Comment 23 Sebastian Dröge (slomo) 2017-03-16 15:38:14 UTC
Created attachment 348100 [details] [review]
video: Add I420/I422/Y444_12LE/BE and GBRA video formats
Comment 24 Sebastian Dröge (slomo) 2017-03-16 15:38:20 UTC
Created attachment 348101 [details] [review]
video-format: Add Since markers to all new formats from 1.2
Comment 25 Sebastian Dröge (slomo) 2017-03-16 16:55:55 UTC
Attachment 348093 [details] pushed as 2fcab9e - video: Add GBRA_10LE/BE, GBR_12LE/BE, GBRA_12LE/BE pixel formats
Attachment 348100 [details] pushed as 77f802f - video: Add I420/I422/Y444_12LE/BE and GBRA video formats
Attachment 348101 [details] pushed as 95ddfde - video-format: Add Since markers to all new formats from 1.2
Comment 26 Sebastian Dröge (slomo) 2017-03-16 16:56:34 UTC
Attachment 348099 [details] pushed as fecf973 - avcodecmap: Add mappings for I420/I422/Y444_12LE/BE and GBRA
Comment 27 Sebastian Dröge (slomo) 2017-03-17 11:50:53 UTC
Some further fixups:

commit ce9bac465a4b088be42efba8e8d4478214f80901
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Fri Mar 17 13:43:04 2017 +0200

    video-format: Shift correctly when packing I420_12BE

commit eda8bd4de122b5ff5605462f0b6c3f46c49ae316
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Fri Mar 17 13:14:58 2017 +0200

    video-format: Order all formats in GST_VIDEO_FORMATS_ALL like in the enum
    
    And remove duplicated entries.
Comment 28 GStreamer system administrator 2018-11-03 11:56:12 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/349.