GNOME Bugzilla – Bug 723289
cairooverlay: add RGB16 support
Last modified: 2014-02-02 13:23:48 UTC
Created attachment 267646 [details] [review] cairooverlay: add support for RGB16 * actual result: On RPI it's currently not possible to do gst-launch-1.0 videotestsrc ! "video/x-raw, format=RGB16" ! cairooverlay ! "video/x-raw, format=RGB16" ! ximagesink and ximagesink reports to support GST_VIDEO_FORMAT_RGB16 (It's intended to work with gst-omx+resize component but easier to report something with videotestsrc) * other infos: In the following patch that's a bit weird to have: #if G_BYTE_ORDER == G_LITTLE_ENDIAN #define TEMPLATE_CAPS GST_VIDEO_CAPS_MAKE("{ RGB16, BGRx, BGRA }") #else #define TEMPLATE_CAPS GST_VIDEO_CAPS_MAKE("{ BGR16, xRGB, ARGB }") Shouldn't be BGR16 on the first line and RGB16 on the second ? But ximagesink report RGB16 and videotestsrc ! ximagesink shows the right color. (if ximagesink reports wrong then that would mean videotestsrc generates the wrong image) Let me know what you think. Also format like RGBx and xBGR have no equivalent in cairo ?
Review of attachment 267646 [details] [review]: ::: ext/cairo/gstcairooverlay.c @@ +101,2 @@ #else +#define TEMPLATE_CAPS GST_VIDEO_CAPS_MAKE("{ BGR16, xRGB, ARGB }") It's RGB16 for both cases because RGB16 is native-endianness in GStreamer. For the other two it's correct this way. And there are no other formats in cairo.
Also, please make it the last format in the list since it's the lowest quality one and the least common one.
Created attachment 267719 [details] [review] cairooverlay: add support for RGB16 (In reply to comment #1) (In reply to comment #2) Done I also changed the default case (in the switch ) as I think it's better to just fail at this point. But then I wonder if I should use GST_ELEMENT_ERROR here instead of a GST_WARNING.
I think an assertion would also be fine. If you get in the default case, GStreamer core did something wrong or there's a really bad bug in the element.
Review of attachment 267719 [details] [review]: ::: ext/cairo/gstcairooverlay.c @@ +166,3 @@ + GST_WARNING ("No matching cairo format for %s", + gst_video_format_to_string (GST_VIDEO_FRAME_FORMAT (frame))); + return GST_FLOW_ERROR; So I think this is ok, but could as well be an assertion. It just must not happen unless something is really really wrong elsewhere already :)
(In reply to comment #5) > Review of attachment 267719 [details] [review]: > > ::: ext/cairo/gstcairooverlay.c > @@ +166,3 @@ > + GST_WARNING ("No matching cairo format for %s", > + gst_video_format_to_string (GST_VIDEO_FRAME_FORMAT (frame))); > + return GST_FLOW_ERROR; > > So I think this is ok, but could as well be an assertion. It just must not > happen unless something is really really wrong elsewhere already :) ok
Looks like this was committed: commit 90b01fce6156359b0787172b380f51cb63c15c98 Author: Julien Isorce <julien.isorce@collabora.co.uk> Date: Fri Jan 31 14:17:54 2014 +0000 cairooverlay: add support for RGB16 https://bugzilla.gnome.org/show_bug.cgi?id=723289