Created attachment 224533 [details] [review]
Don't let GstCollectPads shadow custom sinkpad query function

The first issue I found is that the custom sink pad function was never called. This was due to GstCollectPads overriding it, and chaining to GstPad default. I have solved this issue by setting the custom query function on the GstCollectPads object.

This is not a complete fix for this issue. This negotiation method seems racy and possibly incomplete. Here is some pipeline that fails, or fails after some tries:


gst-launch-1.0 videotestsrc ! video/x-raw,format=AYUV ! videomixer name=mix ! videoconvert ! xvimagesink videotestsrc pattern=1 ! alpha ! mix.

gst-launch-1.0 videotestsrc ! video/x-raw,format=ARGB ! videomixer name=mix ! videoconvert ! xvimagesink videotestsrc pattern=1 ! alpha ! mix.

Makes sense, so committed:

commit 76da367ecde2775df69550de549fb66558992ef1
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Mon Sep 17 13:17:00 2012 -0400

    videomixer: Don't let GstCollectPad shadow custom sink pad query func
    
    In the current implementation, the custom pad query function is not called.
    This patch, set that query function on the GstCollectPads to avoid this
    shadowing.
    
    See https://bugzilla.gnome.org/show_bug.cgi?id=684237

Also mildly related tweaks:

commit 8c28a60eee9b2e1b15ea7c46c3ce24e0bf74b1ed
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Tue Sep 18 12:12:39 2012 +0200

    videomixer: chain up to collectpads query function

commit eda9c8b3cf8cccbadfce9cd84585905568e94b31
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Tue Sep 18 12:13:21 2012 +0200

    videomixer: init videoinfo
    
    ... to prevent random bogus caps fields.

I believe the remaining problem is as explained as follows.

A few relevant caps when it works:
/GstPipeline:pipeline0/GstVideoTestSrc:videotestsrc1.GstPad:src: caps = video/x-raw, format=(string)AYUV, width=(int)320, height=(int)240, framerate=(fraction)30/1, pixel-aspect-ratio=(fraction)1/1
/GstPipeline:pipeline0/GstVideoMixer2:mix.GstPad:src: caps = video/x-raw, format=(string)AYUV, width=(int)320, height=(int)240, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, colorimetry=(string)bt601, framerate=(fraction)30/1
/GstPipeline:pipeline0/GstVideoTestSrc:videotestsrc0.GstPad:src: caps = video/x-raw, format=(string)AYUV, width=(int)320, height=(int)240, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, colorimetry=(string)bt601, framerate=(fraction)30/1

Note that one videotestsrc started out with 'minimal' caps fields, then mixer src pad added some fields (as created by standard videoinfo), and these got propagated back to the other videotestsrc producing 'full' caps.

What happens in failing run:
videotestsrc1 tries to set 'minimal' caps and these fail to make it through:
0:00:00.107385029 11113      0x2375cf0 DEBUG               GST_CAPS ../../gstreamer/gst/gstpad.c:4696:pre_eventfunc_check:<
alpha0:sink> caps video/x-raw, format=(string)AYUV, width=(int)320, height=(int)240, framerate=(fraction)30/1, pixel-aspect
-ratio=(fraction)1/1 not accepted

And afaics this fails because the above caps are not considered a subset (as checked by the default accept_caps) of the following get_caps returned by alpha (in turn originating from the 'full' caps from videomixer src pad):
video/x-raw, format=(string)AYUV, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, colorimetry=(string)bt601, framerate=(fraction)[ 0/1
, 2147483647/1 ]; video/x-raw, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, framerate=(fraction)[ 0/1, 2147483647/1 ], format=(string){ AYUV, ARGB, BGRA, ABGR, RGBA, Y444, xRGB, BGRx, xBGR, RGBx, RGB, BGR, Y42B, YUY2, YVYU, UYVY, I420, YV12, Y41B }

In summary, it seems to come down to caps not considered a subset of caps with some more fields.  So, maybe videomixer has to strip the returned get_caps some more.  Btw, any ideas why videomixer has a separate accept_caps that seems to do what the default one does anyway ?

The custom accept caps is useless indeed, I had the video info init fix too in my branch ;) This seems to be one issue, but there is a bigger one, which is a race. Bascially, when you have more then 1 source, all negotiation happens in parallel. This mean they may all get the most generic caps out of query_caps before one caps get set. This is because the negotiation is not atomic (up to 3 separate operations, query_caps, query_acceptcaps, event_caps). And to make it worst, the setcaps operation in that element lock/unlock 3 times (leaving more room for races). This cannot be fully fixed in the actual design, instead we have to detect such race (uppon caps event) and send reconfigure. We could also consider querying upstream caps as return value of query_caps.

Those are indeed not all atomic and allows races, but I do not see the problem so much in the current design, but within videomixer2 itself.

That is, it returns getcaps with possibly "extra fields", for which it/we know that those might lead to this caps subset problem further upstream.  On the other hand, its own checks in setcaps only check for equal format and par.

So, if the latter (freedom) is indeed what it can handle, it should report as such in getcaps (by definition) (and not have additional fields/restrictions in there).  If it can not handle it, then setcaps is at fault to accept it.

(In reply to comment #5)
> Those are indeed not all atomic and allows races, but I do not see the problem
> so much in the current design, but within videomixer2 itself.

We'll I've been on that with Olivier Crête for few days (there is similar issues in liveadder and adder element, probably all N-1 elements). And it's a really hard problem. At the moment, we focus on getting the initial negotiation to work, renegotiation seems olmost impossible.

> 
> That is, it returns getcaps with possibly "extra fields", for which it/we know
> that those might lead to this caps subset problem further upstream.  On the
> other hand, its own checks in setcaps only check for equal format and par.

Good, point, the custom accept caps is probably to help with that.

> 
> So, if the latter (freedom) is indeed what it can handle, it should report as
> such in getcaps (by definition) (and not have additional fields/restrictions in
> there).  If it can not handle it, then setcaps is at fault to accept it.

I agree, the get caps might not be perfect. But that freedom is also limited, since all the sinkpad must send the same pixel format. What happens is that you have no guaranty that all elements choose the same format (order of caps result in luck most of the time). I do think there exist a solution, but it's going to be complex, and races will be hard to solve. If the negotiation was a single atomic operation, the solution would have been slightly simplier.

Created attachment 224871 [details] [review]
videomixer: Remove unneeded acceptcaps override

    
    The current code is a copy-paste from the getcaps code. Also, it calls
    can_intersect() instead of calling is_subset() has being done in
    the default implementation.

Created attachment 224872 [details] [review]
videomixer: Consider upstream caps in getcaps


    This allow negotiation when a src has format restriction.

Created attachment 224873 [details] [review]
videomixer: Don't add "interlace-mode" and "colorimetry" field


    These field, introduced by gst_video_info_to_caps(), must not be added if
    downstream does not support it. Ohterwise, calls to is_subset() may fail
    when upstream elements try to set caps.

The three last patches should fix the remaining issues. It took me a while to understand the gst_video_info_to_caps() was adding extra fields to the caps set downstream, causing is_subset() to fail in different upstream elements. Reviewer can also pull from:

http://cgit.collabora.com/git/user/nicolas/gst-plugins-good.git/log/?h=videomixer-fixup

It is actually already noted in Comment #3 that those fields 'are created by standard videoinfo'.  As such, there is not really something wrong with having those on the src pad caps (and afaik there could even be more than those removed in the patch).  

It seems/feels somehow more appropriate for getcaps to remove all but the relevant fields (from the src pad caps it starts with), where relevant fields are defined as those that it requires to be equal for all streams (in setcaps) (and where "all but" means preferably not 'hardcoding' the fields to be removed but rather those to keep).

(In reply to comment #11)
> It is actually already noted in Comment #3 that those fields 'are created by
> standard videoinfo'.  As such, there is not really something wrong with having
> those on the src pad caps (and afaik there could even be more than those
> removed in the patch).

It's a convention. Caps set downstream shall be a subset of the caps we query. Videomixer currently add those two fields (via the GstVideInfo utilities). Adding extra fields does not respect the convention.

> 
> It seems/feels somehow more appropriate for getcaps to remove all but the
> relevant fields (from the src pad caps it starts with), where relevant fields
> are defined as those that it requires to be equal for all streams (in setcaps)
> (and where "all but" means preferably not 'hardcoding' the fields to be removed
> but rather those to keep).

I'm not this is a good approach for sink_getcaps(). In the end, the videomixer would endup hiding caps that might be important, or might enable other use cases. I think it's fine for the videomixer to only change the capabilities it knows about and can handle.

On the ohter side, the src_setcaps() could be implemented better by iterating the fields in caps from gst_video_info_to_caps() and fixate the peercaps for fields that actually exist. This would prevent any field to be introduce by videomixer. Not sure how urgent this is though, the attached patches are very good start into the right direction.

(In reply to comment #12)
> (In reply to comment #11)
> > It is actually already noted in Comment #3 that those fields 'are created by
> > standard videoinfo'.  As such, there is not really something wrong with having
> > those on the src pad caps (and afaik there could even be more than those
> > removed in the patch).
> 
> It's a convention. Caps set downstream shall be a subset of the caps we query.
> Videomixer currently add those two fields (via the GstVideInfo utilities).
> Adding extra fields does not respect the convention.

Never heard of this one before (in as far as 'we query' is fairly ambiguous); specified where?  And by adding fields to the caps downstream, they actually are a subset (set-wise) of the upstream caps.

> 
> > 
> > It seems/feels somehow more appropriate for getcaps to remove all but the
> > relevant fields (from the src pad caps it starts with), where relevant fields
> > are defined as those that it requires to be equal for all streams (in setcaps)
> > (and where "all but" means preferably not 'hardcoding' the fields to be removed
> > but rather those to keep).
> 
> I'm not this is a good approach for sink_getcaps(). In the end, the videomixer
> would endup hiding caps that might be important, or might enable other use
> cases. I think it's fine for the videomixer to only change the capabilities it
> knows about and can handle.

hiding (part of) caps ? If they are/were important, then why does the setcaps part not know/care about them.

> 
> On the ohter side, the src_setcaps() could be implemented better by iterating
> the fields in caps from gst_video_info_to_caps() and fixate the peercaps for
> fields that actually exist. This would prevent any field to be introduce by
> videomixer. Not sure how urgent this is though, the attached patches are very
> good start into the right direction.

But anyway ...

From the docs/design/part-negotiation.txt, the recommend way (i.e. convention) is described. You are right that caps with extra field are compatible (let's get rid of the word subset, as it's doing subset of values, not fields). Obviously, the query was a downstream caps query. The way the videomixer is written, introducing new downstream caps, also introduce them upstream. Introducing them upstream will make the capabilities incompatible. Basically, what I'm trying to explain, is that I prefer not introducing new fields in the first place. Trying and workaround it in getcaps seems odd. So far, I haven't seen any other use of gst_video_info_to_caps(), so I can't argue on the right way of using it, maybe it should have had some filter parameter or something. Otherwise, if all those field are consider standard (personnally I have no idea what they are used for), then they should probably be included by GST_VIDEO_CAPS_MAKE() macro no ?

I see nothing in that document that suggests such a "convention" [*].  All it says is that downstream is queried for caps [**] and then the element (i.e. videomixer) still decides what caps to produce, and so it can perfectly decide to produce 'full src caps' (as come up with by videoinfo).  If needed, to stop all this misunderstanding/misinterpretation, please indicate and specify a lot more clearly what "convention" coming from where (precisely).

Furthermore, a search will show that gst_video_info_to_caps *is* used elsewhere, most notably in the videodecoder base class, and so all videodecoder will produce caps that way.  So I see no sense in trying to hack up our own standard API by starting to "patch away" those fields in videomixer src caps but not in a whole lot of other elements, do you? 

And no, those fields do not have to be in GST_VIDEO_CAPS_MAKE, because that is a macro meant for template caps, and those do not need to specify all fields the final actual caps will have (though the actual final caps should have all fields that are specified in template caps).

And what I am also trying to explain, is that it does not matter how videomixer is written *now* (why do we patch stuff?) and there is no law to keep videomixer written as it is, i.e. having all caps fields it comes up with downstream also appear upstream.

And finally, in that regard, videomixer2 is also kind of (pretty) wrong in that when queried for caps it does not consult downstream for caps either (simply takes either current src caps or its own pad template, so not really doing [**]).  This is not optimal since it is *convention* to also consider downstream requirements (e.g. some encoder or whatever).

So, IMO:
* nothing wrong with src caps as produced by video_info_to_caps (as in other elements).  If you really do not like the extra fields (for some reason preferably different than "how it is written now"), then maybe you could query downstream for caps and somehow chuck out fields that are not in those?
* videomixer sink_getcaps could/should also consider downstream caps requirements (yes, in addition then to the upstream ones due to videomixer peculiarities to try and keep all streams compatible)

[*] the only place where the word convention appears is:
- src pad always decides, by convention. sinkpad can suggest a format
     by putting it high in the caps query result GstCaps.
--> (videomixer) src pad decides, so it can have all the video_info_to_caps fields in there it wants

That thread seems to go way outside the goal. Convention was probably a wrong word, which I stated, and yes you are right, one can add field if he wants. I do question the API sometimes, no need to be upset, let's ignore that from now on.

The actual bug, in the videomixer element (which does not happen in a decoder) is that we return the srccaps upstream. This fails when there is multiple sinkpads trying to negotiate at the same moment. Hopefully we are clear on that. My *suggested* fix (the three patches) are incremental fixes to try and address the problem without having to rewrite all the negotiation code. Is your intention to rewrite it yourself ? If so, let me know, I can assume this will be fixed shortly. Otherwise, as I already proposed, I can try and rework the third patches, which seems to be the only one you disagree on, is this right ?

(In reply to comment #10)
> The three last patches should fix the remaining issues. It took me a while to
> understand the gst_video_info_to_caps() was adding extra fields to the caps set
> downstream, causing is_subset() to fail in different upstream elements.
> Reviewer can also pull from:
> 
> http://cgit.collabora.com/git/user/nicolas/gst-plugins-good.git/log/?h=videomixer-fixup

I needed to make one more change to fix the problem in my environment.

In videomixer2.c:gst_videomixer2_pad_sink_getcaps

change:
      gst_structure_set (s, "pixel-aspect-ratio", GST_TYPE_FRACTION, 1, 1,
          NULL);

to:
      gst_structure_set (s, "pixel-aspect-ratio", GST_TYPE_FRACTION_RANGE, 1, G_MAXINT, G_MAXINT, 1, 
          NULL);

(In reply to comment #17)
> (In reply to comment #10)
> 
> I needed to make one more change to fix the problem in my environment.
> 
> In videomixer2.c:gst_videomixer2_pad_sink_getcaps
> 
> change:
>       gst_structure_set (s, "pixel-aspect-ratio", GST_TYPE_FRACTION, 1, 1,
>           NULL);
> 
> to:
>       gst_structure_set (s, "pixel-aspect-ratio", GST_TYPE_FRACTION_RANGE, 1,
> G_MAXINT, G_MAXINT, 1, 
>           NULL);

Good to know, but can we have a bit more context, so we actually understand why is this needed. When I first saw that code, I thought I should have simply removed it, would that also work for you ?

Created attachment 225211 [details]
dot file for mrubinstein's pipeline

I'm decoding an mpeg transport stream.  The video is decoded and alpha blended with graphics frames from an external process.  

See the attached a ".dot" file of the working pipeline.

I tried removing the code that adds the pixel aspect ratio and that works fine.

(In reply to comment #20)
> I'm decoding an mpeg transport stream.  The video is decoded and alpha blended
> with graphics frames from an external process.  
> 
> See the attached a ".dot" file of the working pipeline.
> 
> I tried removing the code that adds the pixel aspect ratio and that works fine.

Thanks a lot, that will help.

*** Bug 688183 has been marked as a duplicate of this bug. ***

I tried applying these patches to current git master. The 3rd one seems to not apply cleanly anymore. Also after applying them then running my test application from bug 688183. I am now getting a weird "ValueError: invalid enum value: 4294967292" error message when trying to run my app.

Created attachment 228961 [details]
Screenshot of cschalle's pipeline

Added screenshot of my pipeline without the patches.

Created attachment 228962 [details]
Correct pipeline screenshot from cschalle

Created attachment 228965 [details]
Screenshot of working pipeline

Ok, worked around my problem based on some help from wtay on IRC. Pasting working pipeline and the chat log with Wim

<cschalle>wtay, hmm, if I am trying to mix two streams with different colorimetry, do I need to use a videoconvert/vidoefilter to make them the same?
<wtay> cschalle, yes
<cschalle>Is the problem that the pipeline is not able to negotiate identical caps between the two src caps ?
<wtay> very likely
<cschalle> wtay, if so, do putting in a fixed caps for the 'videoconverted' stream fix it?
<wtay> yes, you should put a desired output caps
<cschalle> wtay, ok, that solved it. I guess ideally the pipeline should have negotiated this by itself, but this works for now. Thanks a lot
<wtay> we can't reliable negotiate all inputs on a mixer to the same format

Created attachment 229283 [details] [review]
videomixer: Remove unneeded acceptcaps override

The current code is a copy-paste from the getcaps code except that it
calls can_intersect() instead of is_subset(), which is not exact for
accept caps.

Created attachment 229284 [details] [review]
videomixer: Don't override calculated caps

Their is code to determin the best caps, but the result was ignored.

Created attachment 229285 [details] [review]
videomixer: Consider upstream caps in getcaps

This allow negotiation to work when an upstream elements has restrictions.

Created attachment 229286 [details] [review]
videomixer: Don't introduce new fields downstream

These fields, introduced by gst_video_info_to_caps(), must not be added if
downstream does not mention it. Ohterwise, accept_caps event (is_subset())
may fail.

Created attachment 229287 [details] [review]
videomixer: Better use the different locks

There is a specifialised lock to protect the setcaps, especially the saved
video information. We should use this one to protect this structure, and
use the other one to protect the sinkpads list.

I updated the patches, it should be pretty much aligned with Mark's input. There is no more hardcoded fields being removed, so it should work in any conditions now. I have also modified the locking to be more correct, and to prevent potential deadlocks.

Oops, I found that it does not allow mixed resolution anymore (by enable the videomixer test, which was marked broken). I'll fix that, sorry.

Review of attachment 229285 [details] [review]:

One of the issue I just found is that the gst_videomixer2_sinkpads_getcaps() should reset the width and height to a range, so any size can be mixed together. Then I think gst_videomixer2_pad_sink_getcaps() should do a gst_pad_peer_query_caps() on the srcpad somewhere to be correct.

(In reply to comment #34)
> Review of attachment 229285 [details] [review]:
> 
> One of the issue I just found is that the gst_videomixer2_sinkpads_getcaps()
> should reset the width and height to a range, so any size can be mixed
> together. Then I think gst_videomixer2_pad_sink_getcaps() should do a
> gst_pad_peer_query_caps() on the srcpad somewhere to be correct.

Sounds both sensible, yes. But the peer caps should have their width/height removed (we can mix anything, even 1920x1080 to a 320x240 output).

And in any case you need to conserve the PAR that is required downstream and also enforce the same PAR on all sinkpads (same goes for the format field).

Also please do some patch cleanup (or provide a git repository) when you provide a new patch here, it's not obvious to me which patches are still valid and in which order :)

I'm also seeing issues with caps negotiation on other pipelines that worked in 0.10 and don't in 1.0, finding it difficult to narrow down the issue, but testing the first example in http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-good-plugins/html/gst-plugins-good-plugins-videomixer.html produces a not-negotiated error.

*** Bug 700600 has been marked as a duplicate of this bug. ***

I have pending patches on #700600 that seem to be fixing the issue.

>> running
>>    while gst-launch-1.0 videotestsrc num-buffers=1 ! mix. videotestsrc \
>>    num-buffers=1 ! alpha alpha=0.5 ! mix. videotestsrc num-buffers=1 ! mix.\
>>    videomixer name=mix ! videoconvert ! xvimagesink; do echo "hey"; done
> never fails here, could you please send a full debug log to see where it fails

It's a not-negotiated error. This appens in the case the videotestsrc that has no required alpha terminate it's negotiation (sends set caps event) before the one that required alpha. In this case, the new allowed caps become non-alpha fixed caps and the second element fails calling accept caps. I don't see in your patch any code that handle this (if it can be handled at all, latest talk with Sebastian was that there was no solution in 1.0).

In this scenario, the renegotiate event will only be useful if the selected caps has alpha and a non-alpha EVENT comes in, but I think it's useful. Bascially, it saves the day in many cases, but it's not yet a full control over that negotiation process.

Though, it's clear that you patches enable more use case most of the time. I'd suggest bringing your patches here, so we have all of them in one place. I'll try to rebase and cleanup the patches pending here. Patch "Consider upstream caps in getcaps" will be dropped, this has been discussed intensively at the hackfest and Sebastian showed that it may do more bad then good in large pipelines.

Created attachment 244686 [details] [review]
videomixer2: Protect flush_stop_pending with the collectpad stream lock

And make sure to expect a flush-stop after a flush-start

https://bugzilla.gnome.org/show_bug.cgi?id=700600

Created attachment 244687 [details] [review]
videomixer: Do not send flush_stop when receiving a seek

There is no reason to send a flush-stop when receiving a seek event.
In the case of a flushing seek, we could eventually want to, but in
the code path were we check if the seek is "flushing", we have the
following comment that makes sense:

"we can't send FLUSH_STOP here since upstream could start pushing data
after we unlock mix->collect.
We set flush_stop_pending to TRUE instead and send FLUSH_STOP after
forwarding the seek upstream or from gst_videomixer_collected,
whichever happens first."

https://bugzilla.gnome.org/show_bug.cgi?id=700600

Created attachment 244688 [details] [review]
videomixer: Send caps event from the streaming thread

This way we avoid races in caps negotiation and we make sure
that the caps are sent after stream-start.

https://bugzilla.gnome.org/show_bug.cgi?id=700600

Created attachment 244689 [details] [review]
videomixer: Send a reconfigure event upstream if sinkpad caps are not usable

https://bugzilla.gnome.org/show_bug.cgi?id=700600

Created attachment 244690 [details] [review]
tests: Re-enable videomixer test

https://bugzilla.gnome.org/show_bug.cgi?id=700600

Here is my branch with the patches: http://cgit.collabora.com/git/user/tsaunier/gst-plugins-good/log/?h=videomixer

Review of attachment 244686 [details] [review]:

One small comment about this one, but I think it can still me merged.

::: gst/videomixer/videomixer2.c
@@ +963,3 @@
     GST_DEBUG_OBJECT (mix, "pending flush stop");
     gst_pad_push_event (mix->srcpad, gst_event_new_flush_stop (TRUE));
+    mix->flush_stop_pending = FALSE;

My understanding is that this case does not exist, unless all sources forget to send flush-stop and starts sending data. Is that correct ?

Review of attachment 244687 [details] [review]:

Yes, that made no sense to me either.

Review of attachment 244688 [details] [review]:

This seems like a good idea, and it compensate the fact the caps event is being handled in two sequences of lock/unlock, though this should eventually be fixed.

::: gst/videomixer/videomixer2.c
@@ +2071,3 @@
   mix->collect = gst_collect_pads_new ();
   mix->background = DEFAULT_BACKGROUND;
+  mix->current_caps = NULL;

This is optional, gobject are initialize to zero at allocation. Not a blocker to me.

Review of attachment 244689 [details] [review]:

I think this one will need some thought, and maybe second opinion. What I had in mind for when I'd give a second round to this, would have been to keep the filter caps passed durring  caps query, and only send reconfigure if the selected caps are part of the filtered caps. This way, only element with buggy negotiation could loop.

::: gst/videomixer/videomixer2.c
@@ +322,3 @@
+          "current caps are %" GST_PTR_FORMAT, caps, mix->current_caps);
+      gst_pad_push_event (pad, gst_event_new_reconfigure ());
+      return FALSE;

I'm worried about this one because if an element don't call accept_caps() (which is optional iirc) because it only support a single format, it may keep sending and getting reconfigure again and again.

Review of attachment 244690 [details] [review]:

If the reconfigure is not strictly required, go for it.

Attachment 244686 [details] pushed as 85b6852 - videomixer2: Protect flush_stop_pending with the collectpad stream lock
Attachment 244687 [details] pushed as 718f900 - videomixer: Do not send flush_stop when receiving a seek
Attachment 244688 [details] pushed as 86b1060 - videomixer: Send caps event from the streaming thread
Attachment 244690 [details] pushed as f1f149b - tests: Re-enable videomixer test

Comment on attachment 229286 [details] [review]
videomixer: Don't introduce new fields downstream

I don't think this patch is correct, video caps should always contain those fields... and additional fields will not cause accept-caps to fail.

(In reply to comment #47)
> Review of attachment 244686 [details] [review]:
> 
> One small comment about this one, but I think it can still me merged.
> 
> ::: gst/videomixer/videomixer2.c
> @@ +963,3 @@
>      GST_DEBUG_OBJECT (mix, "pending flush stop");
>      gst_pad_push_event (mix->srcpad, gst_event_new_flush_stop (TRUE));
> +    mix->flush_stop_pending = FALSE;
> 
> My understanding is that this case does not exist, unless all sources forget to
> send flush-stop and starts sending data. Is that correct ?

If anything sends data after flush-start and before flush-stop, the pads will reject the data. So that can never happen

(In reply to comment #50)
> Review of attachment 244689 [details] [review]:
> 
> I think this one will need some thought, and maybe second opinion. What I had
> in mind for when I'd give a second round to this, would have been to keep the
> filter caps passed durring  caps query, and only send reconfigure if the
> selected caps are part of the filtered caps. This way, only element with buggy
> negotiation could loop.
> 
> ::: gst/videomixer/videomixer2.c
> @@ +322,3 @@
> +          "current caps are %" GST_PTR_FORMAT, caps, mix->current_caps);
> +      gst_pad_push_event (pad, gst_event_new_reconfigure ());
> +      return FALSE;
> 
> I'm worried about this one because if an element don't call accept_caps()
> (which is optional iirc) because it only support a single format, it may keep
> sending and getting reconfigure again and again.

Why would that ever loop? Upstream would check if it can negotiate something new here, if it can't (or ignores the reconfigure event), at latest stuff will error out when buffers are pushed (because then an incompatible caps event would be forwarded, and core checks with accept-caps if caps are supported before forwarding that event).

(In reply to comment #55)
> Why would that ever loop? Upstream would check if it can negotiate something
> new here, if it can't (or ignores the reconfigure event), at latest stuff will
> error out when buffers are pushed (because then an incompatible caps event
> would be forwarded, and core checks with accept-caps if caps are supported
> before forwarding that event).

Maybe accept_caps() should not be documented as optional anymore, would make stuff less confusing. Asside this, I agree that a element resending the same caps when reconfigure is set while sending caps event is wrong.

(In reply to comment #53)
> (From update of attachment 229286 [details] [review])
> I don't think this patch is correct, video caps should always contain those
> fields... and additional fields will not cause accept-caps to fail.

Here's an example:

-> Branch A choose video/x-raw, but videomixer sets video/x-raw,pixel-aspect-ratio=1/1
-> Branch B choose video/x-raw, which will fail accept caps, since video/x-raw is not a bubset of video/x-raw,pixel...

Using gst_video_info_(from/to)_caps() is what introduce this issue.

(In reply to comment #57)
> (In reply to comment #53)
> > (From update of attachment 229286 [details] [review] [details])
> > I don't think this patch is correct, video caps should always contain those
> > fields... and additional fields will not cause accept-caps to fail.
> 
> Here's an example:
> 
> -> Branch A choose video/x-raw, but videomixer sets
> video/x-raw,pixel-aspect-ratio=1/1
> -> Branch B choose video/x-raw, which will fail accept caps, since video/x-raw
> is not a bubset of video/x-raw,pixel...
> 
> Using gst_video_info_(from/to)_caps() is what introduce this issue.

Where would the caps without pixel-aspect-ratio come from? I think we should hardcode the list of omitted fields though, or better, only put a fixed list of fields into the srcpad caps (everything that must not change and everything that must be provided, i.e. format, pixel-aspect-ratio, ... and width/height).

Comment on attachment 244689 [details] [review]
videomixer: Send a reconfigure event upstream if sinkpad caps are not usable

Attachment 244689 [details] pushed as 18ef4f1 - videomixer: Send a reconfigure event upstream if sinkpad caps are not usable

*** Bug 704852 has been marked as a duplicate of this bug. ***

This has been resolved by adding color convertion to the mixer.

*** This bug has been marked as a duplicate of bug 704950 ***