GNOME Bugzilla – Bug 704950
videomixer: add colorspace conversion
Last modified: 2013-09-18 16:48:12 UTC
Right now videomixer isn't supposed to accept different formats on its sinkpads, and also goes in an infinite loop when trying to do so. The first patch copies videoconvert's code and packs it with videomixer, renaming some symbols in the process to avoid conflict. Worth to note is that the ORC_SOURCE directive doesn't allow to specify multiple source files, so I worked around that by concatenating blendorc.orc and gstvideoconvertorc.orc together in gstvideomixerorc.orc. The correct fix would be to fix ORC_SOURCE but I don't know my autotools very well. The second patch implements colorspace conversion and modifies the _sink_setcaps function of videomixer. What's left to do ? Reconfigure the conversions and the srcpad when pads are released and we can switch to a "less-converting" option or when pads are added with an alpha and the old caps of the srcpad didn't have alpha. I'll attach new patches when this is done but I believe this can be reviewed already.
Created attachment 250227 [details] [review] First patch moving all the code
Created attachment 250228 [details] [review] second patch implementing conversion
Created attachment 250230 [details] [review] New patch that implements conversion I'm sorry, just re read the patch and saw an enormity (frame unmapped before reading from it) it's corrected now. Also a pipeline to demonstrate the patch : gst-launch-1.0 videotestsrc ! video/x-raw, format=I420 ! videomixer name=m sink_0::alpha=0.5 sink_1::alpha=0.5 ! videoconvert ! autovideosink videotestsrc pattern=18 ! video/x-raw, format=RGBA ! m.
This doesn't seem to be the best approach. The biggest issue is the duplication of code from videoconvert, which at best creates a maintenance headache. What benefits does this provide over using multiple videoconvert elements?
The code duplication is temporary until this gets moved into libgstvideo. The benefit would be that you always have a fallback and can deal with the buffer without race conditions.
(In reply to comment #4) > This doesn't seem to be the best approach. The biggest issue is the duplication > of code from videoconvert, which at best creates a maintenance headache. What > benefits does this provide over using multiple videoconvert elements? As __tim said, when the code is moved in libgstvideo we'll make use of it, but that's indeed an issue. The problem with using multiple videoconvert elements before the sinks is that first not everyone puts videoconverts before videomixer, and second that would imply reconfiguring the sinkpads when a new format is requested, and this is racy by nature as the reconfiguring is asynchronous, meaning you might lose frames, we discussed it on the irc and decided that this was indeed the best method. Another issue was to decide what to return in get_caps in the interval, and also make sure we wouldn't go in an infinite reconfiguration loop, as was actually the case until now.
Any reason the videoconvert code can't be moved to a library right now?
Hm I don't know why __tim and slomo said it would have to be done later but a
OOps a problem I see is that you would have to fixate the API, add annotations etcaetera.
(In reply to comment #7) > Any reason the videoconvert code can't be moved to a library right now? I don't feel the videoconvert API is right to be exposed yet as it is. But, I also don't think it's that much work to fix it up.
> Any reason the videoconvert code can't be moved to a library right now? Sebastian and I felt it's something that would best be done in the next cycle and not so shortly before 1.2. If Wim feels like fixing up the API and is confident it will be right, then that's fine too of course.
Good point. I had meant for 1.4.
There's an issue with that patch, as it will only work for same-sized incoming videos. I've fixed it here, the solution seems correct to me, but I have started working on reconfiguration of the converters, which is now done in the case of alpha, but needs some refactoring as I'd like the code to be a little smarter, which means not only reconfiguring in the case of new pads with alpha but also in the case of any addition of pads and any removal of pads, and the output size will have to be modified accordingly. I'll finish that tomorrow hopefully.
Created attachment 250307 [details] [review] This patch implements "smart" conversion and alpha preservation. That patch allows to update the converters when new pads are added or old pads are removed. I have a
Sorry, I have a few questions about it though : I am not sure what to do about the colorimetry field, I believe we can safely remove it in the negotiation functions as we'll do conversion. I also don't know if that's the right way to set the format field. Size calculation works with my test cases, not sure it will in all cases, any opinions ? This patch also doesn't take action when convert_convert_new returns NULL, I was wondering what to do in that case.
(In reply to comment #0) > Right now videomixer isn't supposed to accept different formats on its > sinkpads, and also goes in an infinite loop when trying to do so. First of all this infinite loop should be fixed, independent of anything else.
Comment on attachment 250230 [details] [review] New patch that implements conversion Looks good in general, but this *only* ever does conversion if we run into one of these race conditions, right? E.g. If one pad is configured with RGBA already, but another one tries to configure with I420 and checked what videomixer can do before RGBA was configured? Would it make sense to just always allow conversion now inside videomixer but keep the configured format at the top of the caps that are returned by the CAPS query?
Review of attachment 250307 [details] [review]: ::: gst/videomixer/videomixer2.c @@ +315,3 @@ + /* If we want alpha, disregard all the other formats */ + if (need_alpha && !(pad->info.finfo->flags & GST_VIDEO_FORMAT_FLAG_ALPHA)) + continue; What if downstream does not support an alpha format at all? @@ +317,3 @@ + continue; + + formats[GST_VIDEO_INFO_FORMAT (&pad->info)] += 1; As said on IRC this is not going to work because GST_VIDEO_FORMAT_LAST can change at any point and we can't just require videomixer to be recompiled because of this. @@ +333,3 @@ + mix->info = best_info; + mix->info.width = new_width; + mix->info.height = new_height; You should also check if downstream even supports what you chose here, I think this code should be inside update_src_caps() for that reason @@ +451,1 @@ + gst_structure_set_value (s, "format", v); I think what you want here is first the configured caps (i.e. format, colorimetry, etc but not width/height/framerate) and afterwards almost blank raw video caps to allow anything we can convert into. @@ +451,3 @@ + gst_structure_set_value (s, "format", v); + + gst_structure_remove_field (s, "colorimetry"); There are also other fields that we can convert between. Like chroma-siting. Check videoconvert for the fields you can remove @@ +474,3 @@ + template_caps = gst_pad_get_pad_template_caps (GST_PAD (mix->srcpad)); + s = gst_caps_get_structure (template_caps, 0); + v = gst_structure_get_value (s, "format"); Same goes here, also the *template* caps probably don't have a single format field. You need to intersect the template caps with the downstream accepted caps, and then somehow select suitable caps from those that remain.
I can separate the patch for the infinite loop if needed. Actually, we thought about the format problem with Thibault and realized that the possible formats for the pads being hardcoded, it's not possible to have new formats in videomixer without first recompiling it anyway, so that patch may actually be valid ? I'll have a look at how to set these fields in the correct way, I guess I'll come on IRC to discuss it :)
(In reply to comment #17) > (From update of attachment 250230 [details] [review]) > Looks good in general, but this *only* ever does conversion if we run into one > of these race conditions, right? E.g. If one pad is configured with RGBA > already, but another one tries to configure with I420 and checked what > videomixer can do before RGBA was configured? > > Would it make sense to just always allow conversion now inside videomixer but > keep the configured format at the top of the caps that are returned by the CAPS > query? I'm not sure I understand this comment, the conversion will not only take place in the race condition scenario, but as well when a first pad is configured with a format, buffers flow for a moment then after eg 1 second a new pad is added with a different format there will be conversion, and possibly renegotiation of the output format.
(In reply to comment #19) > I can separate the patch for the infinite loop if needed. Please do, that could also be backported to 1.0 then :) > Actually, we thought about the format problem with Thibault and realized that > the possible formats for the pads being hardcoded, it's not possible to have > new formats in videomixer without first recompiling it anyway, so that patch > may actually be valid ? Well, there's no GST_VIDEO_FORMAT_LAST, is there? :) Otherwise you're indeed right
(In reply to comment #20) > (In reply to comment #17) > > (From update of attachment 250230 [details] [review] [details]) > > Looks good in general, but this *only* ever does conversion if we run into one > > of these race conditions, right? E.g. If one pad is configured with RGBA > > already, but another one tries to configure with I420 and checked what > > videomixer can do before RGBA was configured? > > > > Would it make sense to just always allow conversion now inside videomixer but > > keep the configured format at the top of the caps that are returned by the CAPS > > query? > > I'm not sure I understand this comment, the conversion will not only take place > in the race condition scenario, but as well when a first pad is configured with > a format, buffers flow for a moment then after eg 1 second a new pad is added > with a different format there will be conversion, and possibly renegotiation of > the output format. Well, that's exactly the race condition case. Because you can currently only get a different format on the second pad if the race condition happens. Because after one pad is configured right now the CAPS query will only return caps with the format of that first pad.
Created attachment 250974 [details] [review] This patch avoids infinite reconfiguration loop and should be easy to backport.
Created attachment 250975 [details] [review] This patch fixups the two previous patches, and fix observations made. What would need to be merged now is the patch moving all the code, the one avoiding reconfiguration and this attachment.
I mentioned a needed fix to Sebastian, that I just pushed. I think I'd rather stop posting patches and link my branches, hope it's all right : https://github.com/MathieuDuponchelle/gst-plugins-good/commits/mixer_converter
Added some comments there
Fixed and pushed the comments there : https://github.com/MathieuDuponchelle/gst-plugins-good/commits/mixer_converter_reviewed I didn't do one thing, which was to factor out the pad releasing, as it's a one liner (videomixer_videoconvert_convert_free (pad->convert);) seemed overkill to me.
Added some more comments there
Answered these comments
And added some more. I have to say I prefer the Bugzilla review work-flow :P
Fixed your last comments in https://github.com/MathieuDuponchelle/gst-plugins-good/commit/e974aec706dc36858230f7d8141db42f5f225c31, you can now run pipelines like : gst-launch-1.0 videotestsrc ! video/x-raw, format=I420 ! videomixer name=m sink_0::alpha=0.5 sink_1::alpha=0.5 sink_1::xpos=100 ! video/x-raw, format=AYUV ! videoconvert ! autovideosink videotestsrc pattern=18 ! video/x-raw, format=RGBA ! m.
Actually the commit name isn't correct anymore, it's on top of my mixer_converter_reviewed anyway :)
Some more comments but I think we're almost there
commit 8db40a8c7f1cac4b361414af56887aa7efac4d05 Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Fri Jul 26 19:40:53 2013 +0200 videomixer: Add colorspace conversion https://bugzilla.gnome.org/show_bug.cgi?id=704950 commit 707e39fe7aa1f30f8893a86023323a508e35ff14 Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Tue Aug 6 15:38:39 2013 +0200 videomixer: Don't send reconfigure event when formats or PAR are different It is racy with multiple pads. https://bugzilla.gnome.org/show_bug.cgi?id=704950 commit 8db36485443c767c93c3aebe379a66efa0fd301b Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Thu Jul 25 13:49:57 2013 +0200 videomixer: Bundle private copies of videoconvert code Ideally, this would be part of libgstvideo. Prefixes videoconvert symbols with videomixer_. https://bugzilla.gnome.org/show_bug.cgi?id=704950
*** Bug 684237 has been marked as a duplicate of this bug. ***