GNOME Bugzilla – Bug 767226
audioconvert: Preserve channels when we only need to reposition
Last modified: 2016-06-17 09:46:15 UTC
Pipeline such as: gst-launch-1.0 http://media.xiph.org/BBB/bbb3d/audio/bbb3d_sunflower_soundtrack_surround.ac3 ! ac3parse ! avdec_ac3 ! audioconvert ! opusenc ! fakesink -v are downscaling to 1 channel because the channel-mask between the ac3 file content and what opusenc support do not match, we can convert it so let's just do so.
Created attachment 329103 [details] [review] audioconvert: Preserver channels when we only need to reposition GstAudioConveter is able to reposition channels so we should make sure that we try to keep all the channels and never fail only because of channel positions mismatch.
Created attachment 329104 [details] [review] audioconvert: Preserve channels when we only need to reposition GstAudioConveter is able to reposition channels so we should make sure that we try to keep all the channels and never fail only because of channel positions mismatch.
(pitivi bug: https://phabricator.freedesktop.org/T7414)
Unit test would be nice! :)
Review of attachment 329104 [details] [review]: ::: gst/audioconvert/gstaudioconvert.c @@ +271,3 @@ + + /* We are able to do channel positioning conversion */ + gst_structure_remove_fields (st, "channel-mask", NULL); Except that we are not: bug #666506 This needs to be more fine-grained for the positions that we can actually convert.
Hrm, ok I do not understand that other bug then because while negotiating it does not claim it handle channel positioning conversion at all. I will have a look and add a unit test.
The problem is that we can convert just fine only if: 1) All channels from the input are also in the output (your case I guess?), i.e. we only need to reorder but not mix 2) All channels are in positions where we support mixing (the first 8 positions or so) Otherwise it will fail with great effect :) Currently the caps negotiation does not consider that at all.
> 1) All channels from the input are also in the output (your case I guess?), i.e. we only need to reorder but not mix yeah, this is my case, so I will just force output to have the same number of channels, and that works in that case. I will still try to look at #666506 later.
The same *number* of channels is not the same as the *same* channels :) We only handle the case where it's the same channels in a different order, or the case where it's only those channels we support mixing for. I wouldn't worry too much about bug #666506, just let negotiation fail if it's neither of the two cases. That should be identifiable relatively easily with the mask.
(In reply to Sebastian Dröge (slomo) from comment #9) > The same *number* of channels is not the same as the *same* channels :) We > only handle the case where it's the same channels in a different order, or > the case where it's only those channels we support mixing for. Ah, got it, let's try to be smarter about that then. > 2) All channels are in positions where we support mixing (the first 8 positions or so) What does "the first 8 positions" mean in practice, up to 8 channels?
(In reply to Thibault Saunier from comment #10) > > 2) All channels are in positions where we support mixing (the first 8 positions or so) > > What does "the first 8 positions" mean in practice, up to 8 channels? No, the 8 (or so) positions where we actually implement mixing. That's IIRC - mono - front left - front right - front center - rear left - rear right - rear center - side left - side right But better check the code :)
Basically every layout that has at least one channel that is not one of the above will fail remixing
There was some confusion here. I think what we need first now is a patch to the audioconvert unit tests to show the problem :) This might be bug #706884 So for the confusion. There is exactly one valid *order* of channels in 1.x, if encoders/decoders/etc need something different we have API for conversion but that's internal to the elements. So this is not the problem here. The problem here is, according to Thibault, that opusenc wants 0x3f as channel mask and the input is 0xc0f. The audioconvert between the input and opusenc however is converting to mono instead of the appropriate 6 channel layout. That might be bug #706884 but we don't know for sure yet, needs more debugging/info from Thibault :) The other problem seems to be that audioconvert happily converts channel positions it does not actually know about. The 0xc00 part of the channel mask is IIRC not supported for conversion in audioconvert at all, as such negotiation should fail (for going to mono as well as going to 0x3f). Which is then related to bug #666506.
Created attachment 329797 [details] [review] tests:audioconvert: add test for number of channels negotiation
The first test passes (as we have the same channel positions) but the second one fails as audioconvert negotiates only one channel on it srcpad, basically because it used only the first structure when fixating caps (bug #706884).
Thanks for taking the time to report this. This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find. *** This bug has been marked as a duplicate of bug 706884 ***