After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 421543 - [GstPad] Doesn't check if pad accepts caps after caps change
[GstPad] Doesn't check if pad accepts caps after caps change
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal major
: 0.10.13
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 429319
 
 
Reported: 2007-03-22 16:53 UTC by Sebastian Dröge (slomo)
Modified: 2007-05-21 12:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstcaps.diff (788 bytes, patch)
2007-03-22 16:54 UTC, Sebastian Dröge (slomo)
none Details | Review
gstcaps.diff (3.39 KB, patch)
2007-03-22 17:12 UTC, Sebastian Dröge (slomo)
needs-work Details | Review
caps-compatible.diff (2.17 KB, patch)
2007-03-25 10:52 UTC, Sebastian Dröge (slomo)
none Details | Review
caps-compatible.diff (4.42 KB, patch)
2007-03-25 15:40 UTC, Sebastian Dröge (slomo)
none Details | Review
gstpad.diff (1.20 KB, patch)
2007-03-25 19:30 UTC, Sebastian Dröge (slomo)
none Details | Review
gstpad.diff (1.38 KB, patch)
2007-03-31 17:54 UTC, Sebastian Dröge (slomo)
none Details | Review
gstpad-caps.diff (3.45 KB, patch)
2007-04-12 19:40 UTC, Sebastian Dröge (slomo)
none Details | Review
gstpad-test.diff (2.46 KB, patch)
2007-04-12 19:42 UTC, Sebastian Dröge (slomo)
committed Details | Review
Simplified version (2.61 KB, patch)
2007-05-21 11:49 UTC, Wim Taymans
committed Details | Review

Description Sebastian Dröge (slomo) 2007-03-22 16:53:37 UTC
Hi,
currently GstPad doesn't check if the peer pad accepts the new caps when they changed. Only the setcaps function of the peer pad is called, not gst_pad_accept_caps().

This breaks in the following scenario:

pipeline: A ! B
A has "audio/x-raw-int,width={16,32},depth={16,32}" as static caps template for the src pad.
B has "audio/x-raw-int,width=32,depth={16,32}" as static caps template for the sink pad.

A and B can apparently link to each other.

These caps are not more specified while calling the setcaps function of A's src pad but only when the first buffer is pushed out of A's src pad.
A sets the caps to "audio/x-raw-int,width=16,depth=16" which are incompatible with B's sink pad caps.

Now only the setcaps function of B's src pad is called in gst_pad_configure_sink() and not gst_pad_accept_caps(). As the setcaps function assumes that only caps compatible with the static caps of the pad are set at all it returns TRUE and now the sink pad of B has caps set that are not compatible with B at all. Dataflow works fine though.

Attached is a patch that fixes it for my case (modulo a flacdec bug) but might have other problems.

Bye
Comment 1 Sebastian Dröge (slomo) 2007-03-22 16:54:04 UTC
Created attachment 85117 [details] [review]
gstcaps.diff
Comment 2 Sebastian Dröge (slomo) 2007-03-22 16:57:17 UTC
The same bug might be in gst_pad_configure_src()
Comment 3 Sebastian Dröge (slomo) 2007-03-22 16:58:45 UTC
Maybe gst_pad_set_caps() should simply call gst_pad_accept_caps() before doing anything else as the docs don't mention that the caps should be compatible with the pad.
Comment 4 Sebastian Dröge (slomo) 2007-03-22 17:12:24 UTC
Created attachment 85120 [details] [review]
gstcaps.diff

Patch that does the checking in set_caps and thus makes configure_{src,sink} useless. Commited this after discussion on IRC.

2007-03-22  Sebastian Dröge  <slomo@circular-chaos.org>

	* gst/gstpad.c: (gst_pad_set_caps), (gst_pad_alloc_buffer_full),
	(gst_pad_chain_unchecked), (gst_pad_push):
	Check in set_caps if the caps are compatible with the pad and remove
	two functions that are redundant now. Fixes #421543.
Comment 5 Sebastian Dröge (slomo) 2007-03-22 18:29:17 UTC
Ok, let's reopen this then as it greatly changes the behaviour and could break under some circumstances.

Any idea how this could be really fixed?
Comment 6 Sebastian Dröge (slomo) 2007-03-22 19:21:59 UTC
Hm, the previous patch also fixed something else:
One could link EVERYTHING to a pad if it has a setcaps function which doesn't return FALSE, i.e. if a setcaps function is provided one has to check whether the caps are fitting or not and can't assume that the core does it correctly.
Comment 7 Sebastian Dröge (slomo) 2007-03-25 10:52:42 UTC
Created attachment 85253 [details] [review]
caps-compatible.diff

Attached is a unit test that reproduces the broken behaviour. Once this is fixed it can be committed to prevent regressions
Comment 8 Sebastian Dröge (slomo) 2007-03-25 13:07:45 UTC
Hm, this is just another case of IMHO broken behaviour... new unit test which checks for the "correct" bug will follow soon.
Comment 9 Sebastian Dröge (slomo) 2007-03-25 15:40:17 UTC
Created attachment 85265 [details] [review]
caps-compatible.diff

Ok, here it is...
"test_push_negotiation_setcaps" tests the original bug:
src, sink get compatible, non-fixed caps, are linked and the sink pad gets a setcaps function. Then the src caps are fixed  incompatible with the sink caps and a buffer with no caps is pushed. This should _fail_ with GST_FLOW_NOT_NEGOTIATED as the src pad caps and sink pad caps are now incompatible and no buffer caps assumes that the buffer has the same caps as the src pad.
Then a buffer with caps == src pad caps is pushed. This should fail for the same reason.

"test_push_negotiation" tests another bug (IMHO):
it does exactly the same but with no setcaps function set.


All cases except "no setcaps & buffer has caps" are currently working although they shouldn't.

Wim, are any of my assumptions wrong?
Comment 10 Sebastian Dröge (slomo) 2007-03-25 19:30:27 UTC
Created attachment 85277 [details] [review]
gstpad.diff

This patch fixes at least the case where the buffer caps != NULL.

For the buffer caps == NULL case some special magic has to be done it seems as in the current code caps_changed is set if buffer caps != NULL and buffer caps != pad caps...

There should either be some checking in gst_pad_set_caps() or caps_changed should be determined by a flag that is set whenever the pad's caps are changed in set_caps and the old check...
Comment 11 Sebastian Dröge (slomo) 2007-03-28 12:23:34 UTC
Oh and that patch also doesn't break the unit tests as the old one that always checked in set_caps... I.e. it _could_ be the correct fix for one of the bugs...
Comment 12 Sebastian Dröge (slomo) 2007-03-30 04:52:53 UTC
For bug #421598 I used an workaround in the encoder now, by calling gst_pad_accept_caps() in the setcaps function.
The decoder still suffers from this bug though, but it will only happen when having no audioconvert afterwards.
Comment 13 Sebastian Dröge (slomo) 2007-03-31 17:54:44 UTC
Created attachment 85617 [details] [review]
gstpad.diff

This time without keeping useless local variables...
Comment 14 Wim Taymans 2007-04-12 14:25:32 UTC
ok, testing the patch now to see if there are any regressions or unpleasant effects of increased caps checking.
Comment 15 Sebastian Dröge (slomo) 2007-04-12 15:34:48 UTC
I can't imagine any unpleasant side-effects but I'm working on a patch that fixes the other bug too. It is based on this one though.

The additional things in the new patch will be:
a) accept_caps being called in set_caps IF there is a pad template for this pad. Only in this case it makes sense to verify if the caps that are set are valid and no unit test would break.

b) Set the caps on all output buffers in pad_push, etc. This will really fix the second bug (see the "caps_changed" logic in 4 places).

a) might not be necessary, b) would be only a very low performance degration so it might be better to only do b) for now (additional to the current patch).

What do you think?
Comment 16 Sebastian Dröge (slomo) 2007-04-12 19:40:38 UTC
Created attachment 86256 [details] [review]
gstpad-caps.diff

New patch that does what I described above...
Comment 17 Sebastian Dröge (slomo) 2007-04-12 19:42:29 UTC
Created attachment 86257 [details] [review]
gstpad-test.diff

...and the unit test with a few bugfixes. With the new patch the expected behaviour happens.

Please review :)
Comment 18 Sebastian Dröge (slomo) 2007-05-19 07:03:36 UTC
Ok, so any new comments or thoughts on this? I would really like to get this or parts of it committed for the release.

IMHO all parts except the first hunk (the set_caps change) are pretty clear, the set_caps part might have some unwanted side effects but I'm not aware of any and it works fine for me since ages now.
Comment 19 Wim Taymans 2007-05-21 11:49:34 UTC
Created attachment 88537 [details] [review]
Simplified version

this patch is rather simple and just always calls acceptcaps, regardless if there is a setcaps function.
Comment 20 Wim Taymans 2007-05-21 12:00:58 UTC
        * gst/gstpad.c: (gst_pad_get_caps_unlocked),
        (gst_pad_acceptcaps_default), (gst_pad_configure_sink),
        (gst_pad_configure_src):
        Added simple version of improved caps checking. It was previously 
        assumed that a setcaps function would check the validity of the caps but  
        people prefer us to check caps against the template automatically. 
        Fixes #421543.
Comment 21 Sebastian Dröge (slomo) 2007-05-21 12:07:44 UTC
Ok, unit test is committed too now. As all other parts of my patches were based on wrong assumptions (elements are just broken if they push buffers with NULL caps after changing caps on the pad) or not necessary (always checking in set_caps).

2007-05-21  Sebastian Dröge  <slomo@circular-chaos.org>

	* tests/check/gst/gstpad.c: (GST_START_TEST), (gst_pad_suite):
	Add unit test for the improved caps checking from bug #421543.