GNOME Bugzilla – Bug 708633
audiomixer: Should not take channel mask in consideration when in mono or stereo
Last modified: 2018-11-03 11:26:03 UTC
Created attachment 255577 [details] [review] fixes the reported issue It can cause negotiation to fail.
Review of attachment 255577 [details] [review]: ::: gst/adder/gstadder.c @@ +293,3 @@ + + s = gst_caps_get_structure (result, i); + if (gst_structure_get_int (s, "channels", &channels)) This could be a range or list, not necessarily an int @@ +348,3 @@ + + n = gst_caps_get_size (caps); + for (i = 0; i < n; i++) { In setcaps the size should always be 1, no? @@ +352,3 @@ + + s = gst_caps_get_structure (caps, i); + if (gst_structure_get_int (s, "channels", &channels)) And here it should be always an int
And with 2 channels the channel mask is important, only with 1 channel it can be omitted (=> mono).
Created attachment 255582 [details] [review] Review fixes. > This could be a range or list, not necessarily an int I think if it's a range we can assume channel-mask should either not be set or not matter here ? > In setcaps the size should always be 1, no? True, fixed > And here it should be always an int OK, but may'be it's still better to check in case it hasn't been set at all ? I know channels is mandatory but some elements might not respect it ?
Shouldn't gst_audio_info_from_caps just ignore the channel mask if the channels={1,2} ? For the setcaps part ?
I think audio_info_is_equal could ignore it in fact, but that's a more intrusive change than the one proposed in my opinion, not sure we want to do that just before release.
Created attachment 255592 [details] [review] This patch makes sure we remove the channel mask even if the "channels" field is a range or a list.
Review of attachment 255592 [details] [review]: Looks good to me
After 1.2.0
commit f330c014121590a5fad01fd06aef1273812b3315 Author: MathieuDuponchelle <mathieu.duponchelle@epitech.eu> Date: Sun Sep 15 21:48:43 2013 +0200 adder: Don't take channel mask in consideration in mono or stereo This could cause negotiation to fail. https://bugzilla.gnome.org/show_bug.cgi?id=708633
Actually this is not correct AFAIU. This would allow mixing two or one channels that are not stereo or mono (i.e. just the rear right channel) with mono.
Mathieu, would have been nice if you had followed up on this... After 3 years it probably doesn't make sense to revert this anymore now, so let's just close this again and point anyone to audiomixer.
Same code was used in audiomixer apparently, so re-open and retitle, since it seems more likely anyone is going to look at that for audiomixer. Test case would probably be helpful.
Automated removal of (bad) usage of the "NONE" target milestone.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/90.