GNOME Bugzilla – Bug 787820
glvideomixer: need update output geo after src caps reconfigure
Last modified: 2017-09-21 02:16:34 UTC
We need reconfigure output geometry when sink caps changed which will occurs once fixate_caps. When do aggregating, check if we have reconfigured src caps. In addition, fixate_caps cannot use gst_structure_fixate_field_nearest_* to update caps because when structure is fixed, it just return false instead updating the value. code in gst/gststructure.c 2174 if (G_VALUE_TYPE (value) == G_TYPE_INT) { 2175 /* already fixed */ 2176 return FALSE;
Created attachment 359963 [details] [review] glvideomixer: need update output geometry after src caps reconfigure
Review of attachment 359963 [details] [review]: Hmm, I suspect that the gst_structure_fixate_field_nearest_int() this will then fail on: videotestsrc ! video/x-raw,width=320,height=240 ! glvideomixer ! video/x-raw(ANY),width=800,height=600 ! fakesink which is a behaviour change. If we want to chagne it, we also need to change the compositor element. ::: ext/gl/gstglvideomixer.c @@ +1081,3 @@ } } + mix->output_geo_changed = TRUE; This should be in the GstGLMixerClass::set_caps() implementation.
(In reply to Matthew Waters (ystreet00) from comment #2) > Review of attachment 359963 [details] [review] [review]: > > Hmm, I suspect that the gst_structure_fixate_field_nearest_int() this will > then fail on: > > videotestsrc ! video/x-raw,width=320,height=240 ! glvideomixer ! > video/x-raw(ANY),width=800,height=600 ! fakesink > > which is a behaviour change. If we want to chagne it, we also need to > change the compositor element. You mean which compositor element? > > ::: ext/gl/gstglvideomixer.c > @@ +1081,3 @@ > } > } > + mix->output_geo_changed = TRUE; > > This should be in the GstGLMixerClass::set_caps() implementation. Yes, I will update it
(In reply to Haihua Hu from comment #3) > You mean which compositor element? https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/compositor
(In reply to Matthew Waters (ystreet00) from comment #4) > (In reply to Haihua Hu from comment #3) > > You mean which compositor element? > > https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/compositor BTW， why it will failed on this： videotestsrc ! video/x-raw,width=320,height=240 ! glvideomixer ! video/x-raw(ANY),width=800,height=600 ! fakesink And I didn't find GstGLMixerClass::set_caps() implementation, do you mean negotiated_caps
(In reply to Haihua Hu from comment #5) > (In reply to Matthew Waters (ystreet00) from comment #4) > BTW， why it will failed on this： > videotestsrc ! video/x-raw,width=320,height=240 ! glvideomixer ! > video/x-raw(ANY),width=800,height=600 ! fakesink Because best_width/height will contain some combination of the input frames and blanket overriding them will cause video/x-raw,width=320,height=240 to be set on the output src pad which does not intersect with downstream's video/x-raw(ANY),width=800,height=600 and thus will fail to negotate. > And I didn't find GstGLMixerClass::set_caps() implementation, do you mean > negotiated_caps The glvideomixer implementation of GstGLMixerClass::set_caps() at https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/gl/gstglvideomixer.c#n870
(In reply to Matthew Waters (ystreet00) from comment #6) > (In reply to Haihua Hu from comment #5) > > (In reply to Matthew Waters (ystreet00) from comment #4) > > BTW， why it will failed on this： > > videotestsrc ! video/x-raw,width=320,height=240 ! glvideomixer ! > > video/x-raw(ANY),width=800,height=600 ! fakesink > > Because best_width/height will contain some combination of the input frames > and blanket overriding them will cause video/x-raw,width=320,height=240 to > be set on the output src pad which does not intersect with downstream's > video/x-raw(ANY),width=800,height=600 and thus will fail to negotate. Maybe we can do something like that when width and height is fixed, we do not update it, otherwise update it with our best width and height. > > > And I didn't find GstGLMixerClass::set_caps() implementation, do you mean > > negotiated_caps > > The glvideomixer implementation of GstGLMixerClass::set_caps() at > https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/gl/ > gstglvideomixer.c#n870
(In reply to Haihua Hu from comment #7) > (In reply to Matthew Waters (ystreet00) from comment #6) > Maybe we can do something like that when width and height is fixed, we do > not update it, otherwise update it with our best width and height. That is literally the entire point of gst_structue_fixate_nearest_int() ;)
(In reply to Matthew Waters (ystreet00) from comment #8) > (In reply to Haihua Hu from comment #7) > > (In reply to Matthew Waters (ystreet00) from comment #6) > > Maybe we can do something like that when width and height is fixed, we do > > not update it, otherwise update it with our best width and height. > > That is literally the entire point of gst_structue_fixate_nearest_int() ;) Oh, sorry. I made the mistake just then. It's hard to tell whether it is downstream caps or src caps reconfigure
(In reply to Matthew Waters (ystreet00) from comment #8) > (In reply to Haihua Hu from comment #7) > > (In reply to Matthew Waters (ystreet00) from comment #6) > > Maybe we can do something like that when width and height is fixed, we do > > not update it, otherwise update it with our best width and height. > > That is literally the entire point of gst_structue_fixate_nearest_int() ;) BTW, do we really need to support this kind case? Is there any use case? videotestsrc ! video/x-raw,width=320,height=240 ! glvideomixer ! video/x-raw(ANY),width=800,height=600 ! fakesink
(In reply to Matthew Waters (ystreet00) from comment #8) > (In reply to Haihua Hu from comment #7) > > (In reply to Matthew Waters (ystreet00) from comment #6) > > Maybe we can do something like that when width and height is fixed, we do > > not update it, otherwise update it with our best width and height. > > That is literally the entire point of gst_structue_fixate_nearest_int() ;) I notice there is a description of the videoaggregator specification on https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad-libs/html/GstVideoAggregator.html Description VideoAggregator can accept AYUV, ARGB and BGRA video streams. For each of the requested sink pads it will compare the incoming geometry and framerate to define the output parameters. Indeed output video frames will have the geometry of the biggest incoming video stream and the framerate of the fastest incoming one.
(In reply to Haihua Hu from comment #11) > I notice there is a description of the videoaggregator specification on > https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad- > libs/html/GstVideoAggregator.html > > Description > > VideoAggregator can accept AYUV, ARGB and BGRA video streams. For each of > the requested sink pads it will compare the incoming geometry and framerate > to define the output parameters. Indeed output video frames will have the > geometry of the biggest incoming video stream and the framerate of the > fastest incoming one. Which is all overrideable so not really a strong argument for conforming to a certain behaviour.
(In reply to Matthew Waters (ystreet00) from comment #12) > (In reply to Haihua Hu from comment #11) > > I notice there is a description of the videoaggregator specification on > > https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad- > > libs/html/GstVideoAggregator.html > > > > Description > > > > VideoAggregator can accept AYUV, ARGB and BGRA video streams. For each of > > the requested sink pads it will compare the incoming geometry and framerate > > to define the output parameters. Indeed output video frames will have the > > geometry of the biggest incoming video stream and the framerate of the > > fastest incoming one. > > Which is all overrideable so not really a strong argument for conforming to > a certain behaviour. But Maybe let videoaggregator decide output size is more reasonable. When sinkcaps has changed, but glvideomixer output caps cannot been reconfigured even no capsfilter downstream. It's more import to repest the calculated best width and height. What's your opinion? Or do you have some solution to support both kind of case
Created attachment 360045 [details] glvideomixer-dynamic-input Here's a program that does a dynamic input change and successfully updates glvideomixer's output caps with the correct sizes. I'm not sure what issue you're seeing but it does not seem to be an issue with glvideomixer's output size choice. Could you explain a bit your scenario and what doesn't work for you?
(In reply to Haihua Hu from comment #13) > But Maybe let videoaggregator decide output size is more reasonable. When > sinkcaps has changed, but glvideomixer output caps cannot been reconfigured > even no capsfilter downstream. It's more import to repest the calculated > best width and height. What's your opinion? Or do you have some solution to > support both kind of case videoaggregator doesn't know the output size as it's not only used for classical video mixers. Take glmosaic for example which places up to 6 streams on each side of a cube. How would videoaggregator know the output size from that without asking the subclass?
(In reply to Matthew Waters (ystreet00) from comment #14) > Created attachment 360045 [details] > glvideomixer-dynamic-input > > Here's a program that does a dynamic input change and successfully updates > glvideomixer's output caps with the correct sizes. I'm not sure what issue > you're seeing but it does not seem to be an issue with glvideomixer's output > size choice. > > Could you explain a bit your scenario and what doesn't work for you? Full log attached: 1. output caps didn't change on my side using your unit test. when capsfilter change its size, glvideomixer src caps log as below. The rootcause is that when fixate caps, glvideomixer calculate best width and height and try to update caps, but the width and height is fixed to 320*240 when start. Then gst_structure_fixate_field_nearest_int just return false. 0:00:07.225278250 22668 0x122b2ad0 DEBUG glvideomixer gstglvideomixer.c:1092:_fixate_caps: best width 640 best_height 480 0:00:07.225394875 22668 0x122b2ad0 DEBUG videoaggregator gstvideoaggregator.c:786:gst_video_aggregator_update_src_caps:<mixer> to video/x-raw(memory:GLMemory), width=(int)320, height=(int)240, framerate=(fraction)2/1, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, format=(string)RGBA, texture-target=(string)2D 0:00:07.225551875 22668 0x122b2ad0 INFO videoaggregator gstvideoaggregator.c:547:gst_video_aggregator_src_setcaps:<mixer:src> set src caps: video/x-raw(memory:GLMemory), width=(int)320, height=(int)240, framerate=(fraction)2/1, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, format=(string)RGBA, texture-target=(string)2D 2. when aggregator, glvideomixer will only update output geometry when pad geometry changed or the first time to call gst_gl_video_mixer_callback see below code, when caps changed, this will not been called: 1491 if (pad->geometry_change || !pad->vertex_buffer) { ..... 1502 /* top-left */ 1503 v_vertices[0] = v_vertices[15] = 1504 2.0f * (gfloat) pad->xpos / (gfloat) out_width - 1.0f; 1505 /* bottom-left */ 1506 v_vertices[1] = v_vertices[6] = 1507 2.0f * (gfloat) pad->ypos / (gfloat) out_height - 1.0f; 1508 /* top-right */ 1509 v_vertices[5] = v_vertices[10] = v_vertices[0] + 2.0f * w; 1510 /* bottom-right */ 1511 v_vertices[11] = v_vertices[16] = v_vertices[1] + 2.0f * h; ..... } If there is something that I misunderstanding, please point out.
Created attachment 360096 [details] glvideomixer-dynamic-input log
(In reply to Haihua Hu from comment #16) > 1. output caps didn't change on my side using your unit test. when > capsfilter change its size, glvideomixer src caps log as below. The > rootcause is that when fixate caps, glvideomixer calculate best width and > height and try to update caps, but the width and height is fixed to 320*240 > when start. Then gst_structure_fixate_field_nearest_int just return false. It changes with master, but not with 1.12.x so something's been changed/fixed since 1.12 :)
(In reply to Matthew Waters (ystreet00) from comment #18) > (In reply to Haihua Hu from comment #16) > > 1. output caps didn't change on my side using your unit test. when > > capsfilter change its size, glvideomixer src caps log as below. The > > rootcause is that when fixate caps, glvideomixer calculate best width and > > height and try to update caps, but the width and height is fixed to 320*240 > > when start. Then gst_structure_fixate_field_nearest_int just return false. > > It changes with master, but not with 1.12.x so something's been > changed/fixed since 1.12 :) Could you provide some log to me, I have no master env to test.
Created attachment 360103 [details] glvideomixer-dynamic-input-master log
(In reply to Matthew Waters (ystreet00) from comment #20) > Created attachment 360103 [details] > glvideomixer-dynamic-input-master log I setup a latest environment for master， seems that output caps can change as we think. But glvideomixer has issue when compositor, as we talk above, when aggregator, glvideomixer will only update output geometry when pad geometry changed or the first time to call gst_gl_video_mixer_callback see below code, when caps changed, this will not been called: 1491 if (pad->geometry_change || !pad->vertex_buffer) { ..... 1502 /* top-left */ 1503 v_vertices[0] = v_vertices[15] = 1504 2.0f * (gfloat) pad->xpos / (gfloat) out_width - 1.0f; 1505 /* bottom-left */ 1506 v_vertices[1] = v_vertices[6] = 1507 2.0f * (gfloat) pad->ypos / (gfloat) out_height - 1.0f; 1508 /* top-right */ 1509 v_vertices[5] = v_vertices[10] = v_vertices[0] + 2.0f * w; 1510 /* bottom-right */ 1511 v_vertices[11] = v_vertices[16] = v_vertices[1] + 2.0f * h; ..... }
Sure, and for that, your output_geo_changed boolean is fine. The gst_structure_fixate_nearest_int() changes aren't though.
(In reply to Matthew Waters (ystreet00) from comment #22) > Sure, and for that, your output_geo_changed boolean is fine. The > gst_structure_fixate_nearest_int() changes aren't though. Yes, just wait a moment. I need update the patch
Created attachment 360126 [details] [review] glvidemixer: need reconfigure output gemotry after caps renegotiated
commit d6e538dc5651fb03c85d7c7614bcf6c689f2db2f (HEAD -> master) Author: Haihua Hu <jared.hu@nxp.com> Date: Mon Sep 18 15:42:00 2017 +0800 glvideomixer: need update output geometry after src caps reconfigure Need update output geometry when sink caps changed and use gst_structure_set to update caps if structure is fixed https://bugzilla.gnome.org/show_bug.cgi?id=787820
And, this is the correct commit commit d6e538dc5651fb03c85d7c7614bcf6c689f2db2f Author: Haihua Hu <jared.hu@nxp.com> Date: Mon Sep 18 15:42:00 2017 +0800 glvideomixer: need update output geometry after src caps reconfigure Need update output geometry when sink caps changed and use gst_structure_set to update caps if structure is fixed https://bugzilla.gnome.org/show_bug.cgi?id=787820
hmph, I completely screwed this up... Ah well. Here's the correct commit is 379f6bd5d9a16af4053c7dc684fef9fed9ffb382
(In reply to Matthew Waters (ystreet00) from comment #27) > hmph, I completely screwed this up... Ah well. Here's the correct commit is > 379f6bd5d9a16af4053c7dc684fef9fed9ffb382 Thanks! :)