GNOME Bugzilla – Bug 735673
interleave: the example in the documentation does not work
Last modified: 2016-01-13 16:37:18 UTC
Created attachment 284820 [details] [review] Fix examples of use of the interleave element Hi, the example in the documentation of the interleave element didn't work for me when applied to mono files to be interleaved into a stereo stream to be feed to wavenc. It fails with this message: Setting pipeline to PAUSED ... Pipeline is PREROLLING ... ERROR: from element /GstPipeline:pipeline0/GstDecodeBin:decodebin0/GstWavParse:wavparse0: Internal data flow error. Additional debug info: gstwavparse.c(2178): gst_wavparse_loop (): /GstPipeline:pipeline0/GstDecodeBin:decodebin0/GstWavParse:wavparse0: streaming task paused, reason not-negotiated (-4) ERROR: pipeline doesn't want to preroll. Setting pipeline to NULL ... Freeing pipeline ... I guess this is because wavparse used by wavenc gets confused by the missing channel positions configuration. The proper fix when using the element in code would be to set the channel-positions property, but gst-launch-1.0 does not know how to deal with arrays. Setting the channel-mask with capssetter works around the issue. Note that I even tried to set the channel-mask (0x1 and 0x2) to the source streams, this works as well without the need for the capssetter, but I find it misleading as it can induce the user to think that they can control the position of channels in the finals stream by switching the channel masks, which is not true; it looks like the interleave element can only set channel-positions from the input order when used via gst-launch. So IMHO setting channel-mask=0x3 with a capssetter is more adherent to what users should expect to actually happen. Attached there are a set of patches along my line of thoughts. Patches are against 1.4.1 Thanks, Antonio
Review of attachment 284820 [details] [review]: ::: gst/interleave/interleave.c @@ +56,1 @@ * ]| Interleaves two Mono WAV files to a single Stereo WAV file. Please also add a comment explaining the channel-mask issue. Also see bug #733444
After applying #733444, setting the channel-mask manually is not needed anymore when using wavenc, AFAIU wavenc will now use the input position. Should I drop the channel-mask part from my patches? If so I will send a light version of my patchset with just the readability changes and the fixes to the test program. Thanks, Antonio
No, you should still set the channel-mask. Otherwise it will use no useful channel layout here.
Created attachment 286282 [details] [review] fix interleave example, v2 So, let's recap: even if, after bug 73344, interleave and wavenc work fine together when no channel-mask/channel-positions is specified explicitly (wavenc now creates the channel layout from the input channel positions), we cannot assume other encoders will work too as they might require the explicit info, is that correct? Sorry if I am so verbose but it helps me making things clearer in my head. BTW, new patch attached with these changes to the first patch: - reworded the commit message - added a comment to the example pipeline about setting channel-mask Patches are against the 1.4 branch. Thanks, Antonio
Review of attachment 286282 [details] [review]: ::: tests/check/pipelines/wavenc.c @@ +67,1 @@ * Oh I misunderstood, sorry. You should use the channel-positions property, or alternatively make sure that the input has the correct channel positions. In this case you would make sure that they have channels=1,channel-mask=0x1 and channels=1,channel-mask=0x2 Otherwise the output of interleave will have channel-mask=0, which will not be what you usually want.
(In reply to comment #5) > Review of attachment 286282 [details] [review]: > > ::: tests/check/pipelines/wavenc.c > @@ +67,1 @@ > * > > Oh I misunderstood, sorry. You should use the channel-positions property, or > alternatively make sure that the input has the correct channel positions. In > this case you would make sure that they have channels=1,channel-mask=0x1 and > channels=1,channel-mask=0x2 > > Otherwise the output of interleave will have channel-mask=0, which will not be > what you usually want. I agree with you here, in principle, but my point is that the following pipelines produce exactly the same file: gst-launch-1.0 interleave name=i ! \ audioconvert ! wavenc ! filesink location=audio-interleave-test_1.wav \ audiotestsrc wave=0 num-buffers=100 ! audioconvert ! "audio/x-raw,channels=1,channel-mask=(bitmask)0x1" ! queue ! i.sink_0 \ audiotestsrc wave=2 num-buffers=100 ! audioconvert ! "audio/x-raw,channels=1,channel-mask=(bitmask)0x2" ! queue ! i.sink_1 gst-launch-1.0 interleave name=i ! \ audioconvert ! wavenc ! filesink location=audio-interleave-test_2.wav \ audiotestsrc wave=0 num-buffers=100 ! audioconvert ! "audio/x-raw,channels=1,channel-mask=(bitmask)0x2" ! queue ! i.sink_0 \ audiotestsrc wave=2 num-buffers=100 ! audioconvert ! "audio/x-raw,channels=1,channel-mask=(bitmask)0x1" ! queue ! i.sink_1 I would have expected the channels to be swapped, does this make sense? That's why, as I said in the first message, I thought that setting an opaque channel-mask is more adherent to how interleave behaves. I also tried to use the pipelines above with "interleave name=i channel-positions-from-input=0", but then the pipelines fail. So maybe the interleave element should be fixed to fill channel-positions from the input channel-mask? I can take a better look at the code during the week-end.
Yes, please file a bug against interleave. It should swap the channels in the two pipelines you gave above.
Created attachment 288111 [details] [review] fix interleave examples, V3 Attaching version 3. Now that the interleave element has been fixed with regard to channel ordering, we can just define the correct channel masks in the sink pads and the interleave element will reorder the channels as expected. No need for the opaque approach with capssetter anymore.
Created attachment 318740 [details] [review] fix interleave examples, V4 Hi I rebased the previous v3 on top of 1.7.1 Even if the interleave element will be dropped eventually these fixes are still valid for 1.8. Thanks, Antonio
I pushed this: commit be5f94734a57a13294ea06ada1466d041a5654a9 Author: Antonio Ospite <ao2@ao2.it> Date: Fri Aug 29 15:40:23 2014 +0200 tests: fix a thinko in the wavenc example The code is supposed to follow somehow what the comment above says, that is to have one channel with a wave of freq 440 and the other channel with a wave of freq 880, but an off by one error results in frequencies of 0 and 440. https://bugzilla.gnome.org/show_bug.cgi?id=735673 commit bdcc0390aff2b91ceb48fb169e7c155f3e84f4ba Author: Antonio Ospite <ao2@ao2.it> Date: Fri Aug 29 15:07:58 2014 +0200 interleave: Fix the example by setting channel-masks in the sink pads The current example does not work, it fails with: ERROR: from element /GstPipeline:pipeline0/GstDecodeBin:decodebin0/GstWavParse:wavparse0: Internal data flow error. gstwavparse.c(2178): gst_wavparse_loop (): /GstPipeline:pipeline0/GstDecodeBin:decodebin0/GstWavParse:wavparse0: streaming task paused, reason not-negotiated (-4) This is because negotiation with wavenc gets messed up by the missing channel positions configuration. The proper way to define the channel layout when using the interleave element in code would be to set the channel-positions property, but gst-launch-1.0 does not know how to deal with arrays; so the example pipeline works around the issue by setting the channel-masks in the sink pads. Also fix a repetition in the deinterleave example description https://bugzilla.gnome.org/show_bug.cgi?id=735673 but actually I'm confused, because the previous pipeline works just fine for me? It's probably still a good idea to specify the positioning explicitly here though. I've pushed one of the wavenc test patches. About the others: - "tests: fix the wavenc example code to do exactly what its comment says" Not sure what the advantage of this change is? Why generate a file if not needed? - "tests: fix the reference pipeline from the comment in the wavenc test" Doesn't seem to be needed though technically? - "tests: improve comment in the wavenc pipeline test" Should be squashed/combined with previous one, if applied at all. If you attach patches individually, it's easier to track them and comment on them, for what it's worth. I'm going to resolve this since the original issue has been resolved and the rest is cosmetic as far as I can tell, but feel free to follow up.
Comment on attachment 318740 [details] [review] fix interleave examples, V4 Main parts committed.
(In reply to Tim-Philipp Müller from comment #10) > but actually I'm confused, because the previous pipeline works just fine for > me? It's probably still a good idea to specify the positioning explicitly > here though. > It now works here too indeed, I didn't re-test everything for V4. There must have been some wavenc fixes along the way. > I've pushed one of the wavenc test patches. > > About the others: > > - "tests: fix the wavenc example code to do exactly what its comment says" > Not sure what the advantage of this change is? Why generate a file if not > needed? > It was just for consistency with the comment in the file which was mentioning a gst-launch pipeline which created the file. I get your point tho. > - "tests: fix the reference pipeline from the comment in the wavenc test" > Doesn't seem to be needed though technically? > Right, not needed anymore. > - "tests: improve comment in the wavenc pipeline test" > Should be squashed/combined with previous one, if applied at all. > This one was merely to put the sinks to a named element one next to the other. IMHO this way it's easier for newcomers to understand the pipeline. > If you attach patches individually, it's easier to track them and comment on > them, for what it's worth. I'm going to resolve this since the original > issue has been resolved and the rest is cosmetic as far as I can tell, but > feel free to follow up. As you noted the patches are not really needed, so I am not sure if it's worth spending more time on them. Thanks, Antonio
Ok, thanks for the clarification :)