GNOME Bugzilla – Bug 745107
glimagesink: implement GstVideoOverlayCompositionMeta
Last modified: 2015-08-16 13:38:37 UTC
When using this pipeline on android device, I get really bad performance: playbin ! gltransformation ! textoverlay ! glimagesink If I remove the textoverlay the video plays perfectly. As far as I can tell, the problem is that the video buffers are going gpu->cpu->gpu instead of composing the overlay on gpu. According to what people said on #gstreamer, this can be fixed using GstVideoOverlayCompositionMeta.
I can reproduce the performance issue when playing a 4k video. It runs noticeably smoother without the textoverlay. How can a performance gain can be achieved by implementing GstVideoOverlayCompositionMeta? The only place I where this used is tests/check/elements/textoverlay.c. According to the documentation, the raw buffer should be blended over the GL buffer in the sink. How should the pipeline look in this case? As a side note, I would like to see a gl element rendering fonts using this: https://code.google.com/p/glyphy/
The idea being GstVideoOverlayCompositionMeta is to instruct downstream elements to do the overlaying operation. In this case, glimagesink could state it can handle it (return it in the supported meta in the allocation query), and when it sees an incoming buffer with that meta on it upload the overlayed buffer and ask the gpu to blend it. The pipeline wouldn't change (except that the pipeline xavier gave is wrong, I imagine it's s/playbin/decodebin/), textoverlay would figure out that glimagesink can do overlay, and therefore not "burn in" the overlay but instead attach the information in the meta.
We are working on that. We will try to fix it.
*** This bug has been marked as a duplicate of bug 723761 ***
My bad.
kevin_> so you focus on glimagesink implement video overlay composite? <kevin_> do you sync with Julien Isorce? https://bugzilla.gnome.org/show_bug.cgi?id=723761 <kevin_> do you care about gltextoverlay.text_sink? <kevin_> Use an external GL Font API to build a separate gltexture and attach it to the input buffer's GstVideoOverlayCompositionMeta. Then a downstream element could call gst_video_overlay_composition_blend to show up the subtitles. <kevin_> So gltextoverlay still can do video composite with GL even if downstream element can't support GstVideoOverlayCompositionMeta? I also need this use case. <kevin_> how about timeoverlay? do still need gltimeoverlay? <kevin_> so GstBaseTextOverlay based on GL is really needed? <lubosz> i was considering using glyphy for font rendering <lubosz> but we prefer pango atm <lubosz> the bug from julien isorce was marked as duplicate, and later unmarked <lubosz> is there already a gltextoverlay element?? <lubosz> GstBaseTextOverlay won't be based on GL with my work <lubosz> the sink will support compositing the buffers in hardware <lubosz> the font will still be rendered on the cpiu <lubosz> but the textoverlay won't do it's slow compositing anymore
So video composite code based on GL in your work will location in one common fold. So GstBaseTextOverlay based on GL will reuse your code? We need gltextoverlay and gltimeoverlay, and can based on your video composite code?
I'm trying to implement the GST_CAPS_FEATURE_META_GST_VIDEO_OVERLAY_COMPOSITION in another sink using G2D. I added features in static caps template, but besides the GST_CAPS_FEATURE_META_GST_VIDEO_OVERLAY_COMPOSITION feature, GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY also need to be added, since gstbasetextoverlay will use them both for feature negotiation. we need two structure in caps, one is without feature, and one contains feature. if only has one structure with feature, somehow, element link will failed. following if may code clip in my static caps function: -------------------------------------------------------------- caps = gst_caps_new_empty (); for(i=0; i<CAPS_NUM; i++) { GstStructure *structure = gst_structure_from_string(caps_str[i], NULL); gst_caps_append_structure (caps, structure); } for(i=0; i<CAPS_NUM; i++) { GstStructure *structure = gst_structure_from_string(caps_str[i], NULL); gst_caps_append_structure (caps, structure); GstCapsFeatures *f = gst_caps_features_new(GST_CAPS_FEATURE_MEMORY_SYSTEM_MEMORY, NULL); gst_caps_features_add(f, GST_CAPS_FEATURE_META_GST_VIDEO_OVERLAY_COMPOSITION); gst_caps_set_features(caps, i+CAPS_NUM, f); } --------------------------------------------------------------- and now I can get GstVideoOverlayCompositionMeta in may sink.and got the meta info like following: Blending composition 0x71ac4cf8 with 1 rectangles onto video buffer 0x7290f0d8 (1920x1088, format 23) rectangle 0 0x71aa7008: 833x53, format 12 but before all above can be work, we need change video-overlay-composition.c and video-overlay-composition.h a little bit to export GstVideoOverlayComposition and GstVideoOverlayRectangle structures, otherwise we can't get compiled. the patch is attached.
Created attachment 298601 [details] [review] export GstVideoOverlayComposition and GstVideoOverlayRectangle structure export GstVideoOverlayComposition and GstVideoOverlayRectangle structures so that other element can use them to implement a video overlay composition feature
Comment on attachment 298601 [details] [review] export GstVideoOverlayComposition and GstVideoOverlayRectangle structure I don't think that's needed. Why do you think these need to be made public? What can't you get to otherwise?
(In reply to Tim-Philipp Müller from comment #10) > Comment on attachment 298601 [details] [review] [review] > export GstVideoOverlayComposition and GstVideoOverlayRectangle structure > > I don't think that's needed. Why do you think these need to be made public? > What can't you get to otherwise? I have a sink need implement GST_CAPS_FEATURE_META_GST_VIDEO_OVERLAY_COMPOSITION feature, so I use gst_buffer_get_video_overlay_composition_meta() to get the composition meta like following: GstVideoOverlayCompositionMeta *compmeta = gst_buffer_get_video_overlay_composition_meta(buffer); then I need access the data in GstVideoOverlayCompositionMeta. this was defined in video-overlay-composition.h as following: struct _GstVideoOverlayCompositionMeta { GstMeta meta; GstVideoOverlayComposition *overlay; }; then I need access the GstVideoOverlayComposition and rectangles in GstVideoOverlayComposition. but both of them are defined in video-overlay-composition.c, How can I use them in my sink source code?
http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gstvideooverlaycomposition.html See _get_rectangle() and _get_pixels_*().
(In reply to Tim-Philipp Müller from comment #12) > http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base- > libs/html/gst-plugins-base-libs-gstvideooverlaycomposition.html > > See _get_rectangle() and _get_pixels_*(). Thanks a lot, I didn't notice those APIs.
To Kevin: Text overlay are not frequently updated and are small. Rendering text in GL using vectorial data is slower then rendering to a buffer and uploading. So the idea is that we don't care at all about text rendering for this bug. textoverlay or other subtitled element (like assrender) will render to an RGBA buffer and attach it to the buffer to render. All we have to do is upload and compose the buffer in the glimagesink element (and a utility in libgstgl of course, at not everyone uses the sink). This is fully implemented in cogl and clutter video sink.
If only implement video composite in glimagesink, I still don't know how to implement my use case. My use case is v4l2src ! timeoverlay ! videoencoder. How can I accelerate timeoverlay video composite?
(In reply to kevin from comment #15) > If only implement video composite in glimagesink, I still don't know how to > implement my use case. > > My use case is v4l2src ! timeoverlay ! videoencoder. How can I accelerate > timeoverlay video composite? It's a completely different use case, this bug is about glimagesink, and you don't have this element in your pipeline. Please move to the ML if you want to discuss this matter.
These patches implement the GstOverlayComposition API in the glimagesink. The required gst-plugins-base patches can be found here: https://bugzilla.gnome.org/show_bug.cgi?id=751157
Created attachment 305563 [details] [review] Add class for managing GstVideoOverlayCompositionOverlay buffers Add a class to store and manage the OpenGL texture, vertex buffer and GstVideoOverlayRectangle. Transforms overlay coordinate space to vertex buffer space with aspect ratios in mind. Provides methods for caps manipulation and overlay caching. = Example Pipelines = Simple pipeline gst-launch-1.0 videotestsrc ! \ textoverlay text="Hello World" font-desc="sans bold 30" ! \ glimagesink Display 3 static overlays at different positions gst-launch-1.0 videotestsrc ! \ textoverlay text="text1" valignment="top" font-desc="sans bold 30" ! \ textoverlay text="text2" halignment="right" font-desc="sans bold 30" ! \ textoverlay text="text3" halignment="left" font-desc="sans bold 30" ! \ glimagesink Display subtitle file over testsrc gst-launch-1.0 videotestsrc ! \ textoverlay name=foo filesrc location=foo.srt ! subparse ! queue ! foo. foo. ! \ glimagesink
Created attachment 305564 [details] [review] glcolorconvert: Apply GstVideoOverlayCompositionMeta buffer to converted buffer Since glcolorconvert creates a new GstBuffer, without the GstVideoOverlayCompositionMeta data, it needs to be copied to not be dropped.
Created attachment 305566 [details] [review] glimagesink: Add window size event for upstream text overlays This event will propagate the window size upstream, so the basetextoverlay can receive it to render an image buffer according to the devices screen size. This makes text overlays independent of the stream size and reading text over a 320x240 stream on a HiDPI display possible.
Created attachment 305567 [details] [review] glimagesink: Upload and draw overlays Receives the GstOverlayComposition buffer in the glimagesink, manages the GstGLCompositionOverlay objects, caches already uploaded overlays and draws them.
Created attachment 305568 [details] [review] glimagesinkbin: Add allocation query for GstVideoOverlayComposition Adds an GST_VIDEO_OVERLAY_COMPOSITION_META_API_TYPE query to glupload, glcolorconvert and glimagesink. Detects the query from the downstream elements, so it is executed only when downstream supports the overlay API. This makes pipelines with textoverlay ! glupload ! gldownload ! xvimagesink possible.
Created attachment 305569 [details] [review] glimagesinkbin: Add static GstVideoOverlayCompositionMeta caps features
Created attachment 305570 [details] [review] glupload: Move debug init to top of the file
Created attachment 305571 [details] [review] glimagesinkbin: Add dynamic caps for GstVideoOverlayComposition
Created attachment 305572 [details] [review] glupload: Detect overlay meta buffers correctly
Created attachment 305573 [details] [review] glcompositionoverlay: Add compatibility for GL contexts without glGenVertexArrays
Cool stuff. I'm not so sure about this window-size event. I realise it's a quick'n'easy way to make this work, but I think this should be done via the extra GstStructure in the allocation query,which is done anyway to check for the overlay composition meta suppoert, no? glimagesink should then send a reconfigure event if the size changes, not sure that's done yet.
Created attachment 306133 [details] [review] glimagesink: uint is undefined on windows, use guint instead.
@Tim: I managed to pass the window size in the allocation query structure. One challenging aspect is that it has to be passed through glcolorconvert, glupload to textoverlay. It does not need to be passed through the textoverlays weirdly. If I have multiple of them, they all receive the window size from glupload. The major problem about this way of passing the window size is not only the amount of additional code required (why complicated if it can be done simple), but the bug that I receive 3 configure event old resolutions in the textoverlay. So it requires 3 resize / reconfigure events to get the current resolution through all the elements, so the buffer is rendered with the resolution the window hat 3 resize events before. Any solution to this would be appreciated. Maybe reconfigure in the propose_allocation, but this does not sound right. Enjoy: https://git.collabora.com/cgit/user/lubosz/gst-plugins-bad.git/log/?h=videooverlaycomposition-alloc-query-size https://git.collabora.com/cgit/user/lubosz/gst-plugins-base.git/log/?h=videooverlaycomposition-alloc-query-size
Testing your patchset with this pipeline: gst-launch-1.0 videotestsrc ! glupload ! textoverlay text=plop ! glimagesink And the glcontext is leaked when closing the window. If I remove the textoverlay it's not leaked (after the recent patch from Nicolas).
Review of attachment 305563 [details] [review]: ::: gst-libs/gst/gl/gstglcompositionoverlay.c @@ +113,3 @@ + if (GST_GL_IS_CONTEXT (overlay->context)) { + gst_gl_context_thread_add (overlay->context, + gst_gl_composition_overlay_free_vertex_buffer, overlay); The type check is suspicious. Also, what about unrefing the context ;-P
Created attachment 306187 [details] [review] GstGLCompositionOverlay: Fix leaked gl context
That fix the leak I mentioned in comment #31
Review of attachment 305563 [details] [review]: ::: gst-libs/gst/gl/gstglcompositionoverlay.c @@ +299,3 @@ + vmeta = gst_buffer_get_video_meta (comp_buffer); + gst_video_meta_map (vmeta, 0, &info, &raw_overlay_data, &stride, + GST_MAP_READ); Here you map, then you unmap, and keep using the pointer after. Would be nice to explain how this can be is safe ?
Review of attachment 305567 [details] [review]: Any ways this could become a utlity. We need this in gtkglsink too.
Review of attachment 305570 [details] [review]: .
I also think that the window size stuff should be done via the ALLOCATION query. If there are problems with that currently, or if it's too inconvenient, then we should fix that first but don't introduce a new event that is just there to be dropped again in the near future.
Created attachment 306511 [details] [review] glcompositionoverlay: Add class for managing GstVideoOverlayCompositionOverlay buffers
Created attachment 306514 [details] [review] glcompositionoverlay: Add compatibility for GL contexts without glGenVertexArrays
Created attachment 306515 [details] [review] gloverlaycompositor: Add GstGLOverlayCompositor class
Created attachment 306516 [details] [review] glimagesink: Upload and draw overlays with GstGLOverlayCompositor
Created attachment 306517 [details] [review] glimagesinkbin: Add GstVideoOverlayCompositionMeta caps features
Created attachment 306518 [details] [review] glupload: Detect overlay meta buffers correctly
Created attachment 306519 [details] [review] glcolorconvert: Apply GstVideoOverlayCompositionMeta buffer to converted buffer
Created attachment 306520 [details] [review] glimagesinkbin: Add allocation query for GstVideoOverlayComposition
Created attachment 306521 [details] [review] glimagesink: Send reconfigure event when window size changes
Review of attachment 306133 [details] [review]: .
Review of attachment 306187 [details] [review]: .
Review of attachment 306511 [details] [review]: ::: gst-libs/gst/gl/gstglcompositionoverlay.c @@ +34,3 @@ + +#define DEBUG_INIT \ + GST_DEBUG_CATEGORY_INIT (gst_gl_composition_overlay_debug, "glcompositionoverlay", 0, "compositionoverlay"); I suspect this is larger then 80 characters. @@ +41,3 @@ + +void +gst_gl_composition_overlay_add_transformation (GstGLCompositionOverlay * Missing static, put the type on same line unless the gst-indent says otherwise. @@ +279,3 @@ + GST_MAP_READ); + + if (raw_overlay_data != NULL) { I would rather check the return value of gst_video_meta_map(). @@ +312,3 @@ + } + + gst_video_meta_unmap (vmeta, 0, &info); Only unmap if map return TRUE. @@ +323,3 @@ + if (overlay->texture_id != -1) + gl->BindTexture (GL_TEXTURE_2D, overlay->texture_id); + gl->DrawElements (GL_TRIANGLES, 6, GL_UNSIGNED_SHORT, 0); If there is no texture to draw to, isn't it a better idea to simpy not do anything ? ::: gst-libs/gst/gl/gstglcompositionoverlay.h @@ +26,3 @@ +#include <gst/gl/gstgl_fwd.h> + +G_BEGIN_DECLS GType gst_gl_composition_overlay_get_type (void); This is uncommon syntax. I prefer when G_BEGIN_DECLS is on separate line. @@ +40,3 @@ + */ +struct _GstGLCompositionOverlay +{ For documentation you need /*< private >*/ here for opaque type.
Review of attachment 306514 [details] [review]: When the otherone is fixed of course.
I'll mark the one that are ok with commit-now, but read it as these are the ready to ship one, I'll merge everything when we got everything.
Review of attachment 306515 [details] [review]: This patch is duplicated. Same comment applies, could you pick one and cleanup (mark as obsolete) the one you don't want merge.
Review of attachment 306517 [details] [review]: ::: ext/gl/gstglcolorconvertelement.c @@ +54,2 @@ static GstStaticPadTemplate gst_gl_color_convert_element_src_pad_template = + GST_STATIC_PAD_TEMPLATE ("src", Was obviously more readable before, I support you could use INDENT-OFF mark. ::: gst-libs/gst/gl/gstglupload.c @@ +967,2 @@ for (i = 0; i < G_N_ELEMENTS (upload_methods); i++) { + tmp2 = upload_methods[i]->transform_caps (context, direction, caps); You could have kept the tmp2 in the loop scope. I can't see any rational to change this.
Review of attachment 306518 [details] [review]: .
Review of attachment 306519 [details] [review]: .
Review of attachment 306520 [details] [review]: .
Review of attachment 306521 [details] [review]: .
Review of attachment 306516 [details] [review]: .
I'm quite happy with this version. It's much cleaner then the previous ones. As you can see, all my comments are cosmetic, so I believe one more update and we'll be ready to merge this.
Comment on attachment 306133 [details] [review] glimagesink: uint is undefined on windows, use guint instead. This has been squashed in fact.
Comment on attachment 306187 [details] [review] GstGLCompositionOverlay: Fix leaked gl context Was squashed too.
(In reply to Nicolas Dufresne (stormer) from comment #53) > Review of attachment 306515 [details] [review] [review]: > > This patch is duplicated. Same comment applies, could you pick one and > cleanup (mark as obsolete) the one you don't want merge. I was confused, never mind.
Attachment 305570 [details] pushed as 60c8b73 - glupload: Move debug init to top of the file Attachment 306511 [details] pushed as 7c7f84c - glcompositionoverlay: Add class for managing GstVideoOverlayCompositionOverlay buffers Attachment 306514 [details] pushed as b4a4999 - glcompositionoverlay: Add compatibility for GL contexts without glGenVertexArrays Attachment 306515 [details] pushed as b2601a7 - gloverlaycompositor: Add GstGLOverlayCompositor class Attachment 306516 [details] pushed as 778ad10 - glimagesink: Upload and draw overlays with GstGLOverlayCompositor Attachment 306517 [details] pushed as a7d1b7f - glimagesinkbin: Add GstVideoOverlayCompositionMeta caps features Attachment 306518 [details] pushed as 6eaeefc - glupload: Detect overlay meta buffers correctly Attachment 306519 [details] pushed as 5554288 - glcolorconvert: Apply GstVideoOverlayCompositionMeta buffer to converted buffer Attachment 306520 [details] pushed as 2fb862b - glimagesinkbin: Add allocation query for GstVideoOverlayComposition Attachment 306521 [details] pushed as 2b75424 - glimagesink: Send reconfigure event when window size changes
Comment on attachment 306511 [details] [review] glcompositionoverlay: Add class for managing GstVideoOverlayCompositionOverlay buffers I fixed it before merging.
Comment on attachment 306515 [details] [review] gloverlaycompositor: Add GstGLOverlayCompositor class I also fixed it before merging.
This was not ready to be merged for the following reasons. 1. gst-launch-1.0 videotestsrc ! textoverlay text=YAY ! glimagesink and resize the window. The text does not stay in the same place relative to the video. 2. The inconsistent gloverlaycompositor/glcompositionoverlay naming 3. Point 2 should have really been a single file containing both objects, the overall compositor and the GstVideoOverlayComposition wrapper object. Ideally the wrapper object should be entirely internal to the compositor. 4. Reliance on a passed in shader/attributes that we have no idea of the contents of. The compositor should create it's own shader. 5. gst_video_meta_map instead of gst_video_frame_map usage? 6. Not ensuring that the data pointer passed to gst_gl_memory_wrapped is always valid as long the memory is (it may have disappeared at the time of _unmap()). 7. Stride paramater not passed between video_meta_map() and the wrapped GstVideoInfo.
(In reply to Matthew Waters from comment #67) > 1. gst-launch-1.0 videotestsrc ! textoverlay text=YAY ! glimagesink and > resize the window. The text does not stay in the same place relative to the > video. A more correct description is that the overlay lags behind the window resize.
1. The lag is due to the nature of the poorly performing allocation query propagation. It is a known issue. It worked very fluently with a custom upstream event, but the decision was to fix the performance of the allocation query. 2. Which naming do you suggest? 3. Why put 2 classes in 1 file? 4. This would mean more duplication of glimagesink code, but it is doable. 5. - 7. Sounds reasonable.
1. In fact, it's my bad. I forgot to propose the hack for gst-plugins-base to make the negotation fnish every event cycle. https://bugzilla.gnome.org/show_bug.cgi?id=751157 This fixes the lag. It's still slow but the resolution is correct in the same event cycle at least.
(In reply to Lubosz Sarnecki from comment #69) > 1. The lag is due to the nature of the poorly performing allocation query > propagation. It is a known issue. It worked very fluently with a custom > upstream event, but the decision was to fix the performance of the > allocation query. Ok > 2. Which naming do you suggest? Pick one, but be consistent. Moot point with 3). > 3. Why put 2 classes in 1 file? As I see the code use, the compositor directs the usage of the composition objects that is entirely internal to the compositor's operation and doesn't need to be public API. Therefore, the composition object can go into gstgloverlaycompositor.c and not be exposed as public API removing the need for gstglcompositionoverlay.c/h to exist. > 4. This would mean more duplication of glimagesink code, but it is doable. The only thing you're not doing on top of glimagesink is creating/setting up the shader.
2. Let's rename glcompositionoverlay -> gloverlaycompostion 3. We can simply not install the .h also 4. Is that cosmetic ? Or is there a real issue you can demonstrate ? 5. Is cosmetic. It's probably less code, more stack to change it. 7. Need to be looked at indeed. About 1, it can be a bug, e.g. the meta should always be relative to the stream size (regardless of the size it was rendered), but if it's not such a bug, we should pass back the use window size so the sink can handle the difference.
Created attachment 307835 [details] [review] gloverlaycompositor: Create own shader object
Created attachment 307872 [details] [review] glimagesink: Properly handle compsositor life time Should be created in READY_TO_PAUSED, not PAUSED_TO_PLAYING. Should be cleared in PAUSED_TO_READY.
The main issue is that the implementation seems to assume meta coordinate are window size relative, but in fact they should be stream size relative. This will fix the miss-placed overlay. The end result will be correctly placed overlay, with scale up or down overlay until the composition meta get updated. I think this will be fully acceptable (nearly not visible).
Created attachment 307880 [details] [review] gloverlaycompositor: Create own shader object Make gloverlaycompositor independent of the shader used in the sink.
(In reply to Nicolas Dufresne (stormer) from comment #76) > Created attachment 307880 [details] [review] [review] > gloverlaycompositor: Create own shader object > > Make gloverlaycompositor independent of the shader used in the sink. I simply rebased it on top of my patch.
Note, here's few more thing that will need fixing (originally reported by Sebastien, and I didn't understood what he meant). * Handle the case force-aspect-ratio == FALSE * Handle the case where pixel-aspect-ratio != 1/1 This would be pretty much the same in the way to handle. Basically, we need to compute the resulting aspect ratio and use that when placing the overlay. Ideally, we'd pass this pixel-aspect-ratio along with the window width and height so we can get a pre-render that match the display.
(In reply to Nicolas Dufresne (stormer) from comment #78) > Note, here's few more thing that will need fixing (originally reported by > Sebastien, and I didn't understood what he meant). Let me throw another thought in: frame-packed stereo outputs. When rendering out to side-by-side or top-bottom frame packings, if we're blending subpictures, it'll need to be done twice - once for each eye view in the output video. Other frame packings (line-by-line,columns, etc) are probably worse.
Created attachment 307884 [details] [review] composition-overlay: Positions are relative to texture The coordinate are relative to the texture dimension and not the window dimension now. There is no need to pass the window dimension or to update the overlay if the dimension changes.
(In reply to Jan Schmidt from comment #79) > When rendering out to side-by-side or top-bottom frame packings, if we're > blending subpictures, it'll need to be done twice - once for each eye view > in the output video. Other frame packings (line-by-line,columns, etc) are > probably worse. Do you have pipeline and streams, or do you mean you will implement this ?
With the latest patch (and the latest basetextoverlay patch, see bug 751157), it's doing the right thing for the non-3D videos. There is a bug that initial negotiation does not see the overlay meta suppport initially. I'll work on that tomorrow. For 3D support, note that it cannot be done if some combiner is in place. This should already negotiate the textoverlay to render directly. In general, I would not offer the meta if the caps are 3D, so we don't get rushed in figure-out what would be the right thing to do.
Also, Matthew, please confirm the shader change from Lubosz reflects your request.
(In reply to Nicolas Dufresne (stormer) from comment #81) > (In reply to Jan Schmidt from comment #79) > > When rendering out to side-by-side or top-bottom frame packings, if we're > > blending subpictures, it'll need to be done twice - once for each eye view > > in the output video. Other frame packings (line-by-line,columns, etc) are > > probably worse. > > Do you have pipeline and streams, or do you mean you will implement this ? I'll look for or create a usable stereo stream with a suitable subs track for testing. The only one I have isn't share-able.(In reply to Nicolas Dufresne (stormer) from comment #82) > With the latest patch (and the latest basetextoverlay patch, see bug > 751157), it's doing the right thing for the non-3D videos. There is a bug > that initial negotiation does not see the overlay meta suppport initially. > I'll work on that tomorrow. > > For 3D support, note that it cannot be done if some combiner is in place. > This should already negotiate the textoverlay to render directly. In > general, I would not offer the meta if the caps are 3D, so we don't get > rushed in figure-out what would be the right thing to do. Yep - at the moment it gets it horribly wrong there. It'll render over the top of a frame-packed video frame, and then when it gets converted you end up with half the subtitle showing in the wrong place to each eye.
Review of attachment 307880 [details] [review]: Looks good.
This address issue 1 and 4. Attachment 307872 [details] pushed as e8c3161 - glimagesink: Properly handle compsositor life time Attachment 307880 [details] pushed as b155f5d - gloverlaycompositor: Create own shader object Attachment 307884 [details] pushed as 1778815 - composition-overlay: Positions are relative to texture
This address 2 and 3. I believe the naming isn't a big issue now that it's fully private and hidden from the API. commit d92375eaaeff98e267c37c0e9c260f276f2c8289 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Wed Jul 22 14:05:34 2015 -0400 gloverlaycompositor: Hide GstCompsitionOverlay object This object is only used inside the compositor and does not need to be expose in libgstgl API. https://bugzilla.gnome.org/show_bug.cgi?id=745107
This fixes concern 5, 6 and 7. And fixes a regression in Lubosz branch when he added meta parameter. About the 3D stuff, gstglviewconvert, which I believe is needed, does not forward the composition meta, which should lead to textoverlay not attaching it. That's what I expected, please confirm me if I'm wrong. So the bad result is because textoverlay does not support (and does not fail) in presence of 3D view. Sink support for that seems like the step after fixing the software renderer. About the window pixel-aspect-ratio. It would be a nice to have, but not passing it does not result in bug of any kind. The only side, is that the overlay is being scaled, which arguable is something we try to avoid to produce perfect quality subtitle. I'll file a separate bug for that one. commit 4f4aedecf37830e95676cfbb407540e654de9527 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Wed Jul 22 16:58:12 2015 -0400 glupload: Forward composition meta even without params When the sink does not know the window size (e.g not created yet) it will not add any param to the the composition meta. This is no reason not to forward this meta API. Fixes issue where it could not attach until we resize the window. https://bugzilla.gnome.org/show_bug.cgi?id=745107 commit e310e6d5403d703c241fb1fa84977113cb828651 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Wed Jul 22 15:56:34 2015 -0400 gloverlaycompositor: Keep memory pointer alive Keep the composition memory pointer alive while it's being wrapped inside a GstGLMemory object. https://bugzilla.gnome.org/show_bug.cgi?id=745107 commit 9c020443e6959b7c710e6de2ddbee36e9e2a3aab Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Wed Jul 22 14:17:42 2015 -0400 gloverlaycompositor: Pass buffer stride The overlay pixel buffer stride was not given back to the GL image. https://bugzilla.gnome.org/show_bug.cgi?id=745107