GNOME Bugzilla – Bug 660528
kate: rendering performance improvements
Last modified: 2011-12-12 19:46:20 UTC
Rendering onto YUV buffers, and early out where possible.
Created attachment 197831 [details] [review] kate: avoid rendering when we know there is nothing to render
Created attachment 197832 [details] [review] kate: support for rendering on several YUV formats This speeds up rendering a fair bit by not requiring colorspace conversion, whether there is anything to overlay or not. The blending code was nicked from textoverlay. I would think this might be a helpful thing to put in, say, libgstvideo at some point.
Hrmpf. I logged in as my non work email to post this, but git bz seems to have not noticed the cookie had changed. That was done on my own time fwiw.
Created attachment 197928 [details] [review] Update adding YV12, and removing unneeded RGB functions Update adding YV12, and removing unneeded RGB functions
Review of attachment 197928 [details] [review]: ::: ext/kate/gstkatetiger.c @@ +156,3 @@ + +#if G_BYTE_ORDER == G_LITTLE_ENDIAN +# define CAIRO_ARGB_A 3 We're not using cairo here ;) @@ +183,3 @@ #define TIGER_VIDEO_CAPS \ GST_VIDEO_CAPS_xRGB ", endianness = (int)1234; " \ + GST_VIDEO_CAPS_BGRx ", endianness = (int)4321;" \ Erm, you shouldn't specify endianness here. GST_VIDEO_CAPS_xRGB and friends already include this field with the correct value. Note that xRGB with endianness 1234 would be the same as BGRx with endianness 4321. And that we only use big endian endianness for 24/32 bpp RGB formats in the caps anyway and specify the order of components with the masks. @@ +767,3 @@ +static inline void +gst_kate_tiger_blit_1 (GstKateTiger * tiger, guchar * dest, gint xpos, Did you take the YUV rendering functions from another plugin? They look familiar to me :) If you did, maybe add some comment about this here and/or mention copyright holders of the original code, etc. If you didn't copy them, ignore this comment :)
Comment on attachment 197831 [details] [review] kate: avoid rendering when we know there is nothing to render commit fa3d6610833b4b92574a2ef8be870c93b527c5f1 Author: Vincent Penquerc'h <ogg.k.ogg.k@googlemail.com> Date: Thu Sep 29 20:55:22 2011 +0100 kate: avoid rendering when we know there is nothing to render https://bugzilla.gnome.org/show_bug.cgi?id=660528
Review of attachment 197928 [details] [review]: ::: ext/kate/gstkatetiger.c @@ +1228,3 @@ + if (gst_video_format_is_yuv (tiger->video_format)) { + guint8 *tmp = g_realloc (tiger->render_buffer, + tiger->video_width * tiger->video_height * 4); You might want to allocate it once in set_caps... and free it in the PAUSED->READY state change
> +#if G_BYTE_ORDER == G_LITTLE_ENDIAN > +# define CAIRO_ARGB_A 3 > > We're not using cairo here ;) Actually, we are :P But it's kinda hidden, I'll change the naming. > @@ +183,3 @@ > #define TIGER_VIDEO_CAPS \ > GST_VIDEO_CAPS_xRGB ", endianness = (int)1234; " \ > + GST_VIDEO_CAPS_BGRx ", endianness = (int)4321;" \ > > Erm, you shouldn't specify endianness here. GST_VIDEO_CAPS_xRGB and friends > already include this field with the correct value. > > Note that xRGB with endianness 1234 would be the same as BGRx with endianness > 4321. And that we only use big endian endianness for 24/32 bpp RGB formats in > the caps anyway and specify the order of components with the masks. So, xRGB would have the same endianness on both little and big endian machines ? Meaning, a color with 0 everywhere but 255 as red would be not the same value when interpreted as int on both archs ? I want to be really sure as I use Cairo to render, and Cairo stores its colots in host endianness ARGB, so I need to match its format. > Did you take the YUV rendering functions from another plugin? They look > familiar to me :) If you did, maybe add some comment about this here and/or > mention copyright holders of the original code, etc. I nicked them off textoverlay, as the commit message says. I'll add a comment in the code too. Speaking of the commit message, it also mentions these functions could be factored away in libgstvideo. What do you think (I actually have a further patch to make them handle different input stride, which is the version I'd move to libgstvideo) ?
> You might want to allocate it once in set_caps... and free it in the > PAUSED->READY state change Fair point. However, in a future patch, I may well allocate a smaller size first, and a larger size as time goes, without caps changing, based on the estimated render extents, so it'll need to be realloc here, so it might as well stay there if you're OK with that.
(In reply to comment #8) > > @@ +183,3 @@ > > #define TIGER_VIDEO_CAPS \ > > GST_VIDEO_CAPS_xRGB ", endianness = (int)1234; " \ > > + GST_VIDEO_CAPS_BGRx ", endianness = (int)4321;" \ > > > > Erm, you shouldn't specify endianness here. GST_VIDEO_CAPS_xRGB and friends > > already include this field with the correct value. > > > > Note that xRGB with endianness 1234 would be the same as BGRx with endianness > > 4321. And that we only use big endian endianness for 24/32 bpp RGB formats in > > the caps anyway and specify the order of components with the masks. > > So, xRGB would have the same endianness on both little and big endian machines > ? > Meaning, a color with 0 everywhere but 255 as red would be not the same value > when interpreted as int on both archs ? > I want to be really sure as I use Cairo to render, and Cairo stores its colots > in host endianness ARGB, so I need to match its format. The GStreamer definitions are endianness independent. xRGB gives the byte order, i.e. first byte x, second byte R, third byte G and fourth byte B. In Cairo this is slightly different by saying that ARGB has A in the 8 most-significant bits, etc. On little endian systems cairo's ARGB32/RGB24 maps to BGRA, BGRx in GStreamer and on big endian system it maps to ARGB and xRGB. Don't worry about the endianness fields in the caps, it's meaningless for >=24bpp RGB formats. Does this answer your question? :)
Created attachment 198371 [details] [review] kate: support for rendering on several YUV formats This speeds up rendering a fair bit by not requiring colorspace conversion, whether there is anything to overlay or not. The blending code was nicked from textoverlay. I would think this might be a helpful thing to put in, say, libgstvideo at some point.
Thanks for the overview, I've taken out the endianess from the caps. I've looked at the existing overlay lib work in progress, which might include these (or similar) blending routines at some point, so I've left the blending routines as is for now.
commit 9eb79984a88d7eac96806c8b9999f16fa90c80a6 Author: Vincent Penquerc'h <ogg.k.ogg.k@googlemail.com> Date: Thu Sep 29 22:43:30 2011 +0100 kate: support for rendering on several YUV formats This speeds up rendering a fair bit by not requiring colorspace conversion, whether there is anything to overlay or not. The blending code was nicked from textoverlay. I would think this might be a helpful thing to put in, say, libgstvideo at some point. https://bugzilla.gnome.org/show_bug.cgi?id=660528
> The blending code was nicked from textoverlay. I would think > this might be a helpful thing to put in, say, libgstvideo at > some point. fwiw, coming to a bugzilla near you rsn: http://cgit.freedesktop.org/~tpm/gst-plugins-base/tree/gst-libs/gst/video/video-overlay-composition.h?h=video-overlay-composition-textoverlay#n219
Ah, well, I'll switch it to this when available then.
For reference, now using the new video overlay code: commit 83182044af9db3b006970bbcafe55f7f95995436 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Dec 12 19:34:32 2011 +0000 tiger: replace the new YUV blitting code with the newer overlay code