GNOME Bugzilla – Bug 722144
audiodecoder: do not negotiate caps with rate=1 and channels=1 for gap
Last modified: 2014-01-15 18:33:40 UTC
When the first 'data' received by audiodecoder is a gap event it will use a default negotiation procedure that ends up using the fixated version of downstream supported caps. This will often lead to a caps with rate=1 and channels=1. rate=1 is really odd and also crashes pulseaudio. Instead it should use whatever upstream provided in the input caps. The following patches add tests for audiodecoder for this case and also a fix.
Created attachment 266214 [details] [review] tests: audiodecoder: add basic playback test for audio decoder Simple test that just check that audio decoding works as expected
Created attachment 266215 [details] [review] tests: audiodecoder: check that negotiation works buffers and gaps Adds 2 tests to verify that output caps are the expected value, reusing input structure values for both buffers and gaps
Created attachment 266216 [details] [review] audiodecoder: make sure caps is set before forwarding gap event Before trying to generate a default fixated caps when handling a gap event, make sure that the same strategy that is used when handling a buffer has been attempted. Otherwise audiodecoder will ignore upstream caps settings such as rate and channels and will likely end with a caps with channels=1 and rate=1.
Review of attachment 266216 [details] [review]: Looks good, good work :)
Thanks for the quick review commit 8cf8332b91a2caf3b23f711ea1eecd7430a0bfa5 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Mon Jan 13 20:44:23 2014 -0300 audiodecoder: make sure caps is set before forwarding gap event Before trying to generate a default fixated caps when handling a gap event, make sure that the same strategy that is used when handling a buffer has been attempted. Otherwise audiodecoder will ignore upstream caps settings such as rate and channels and will likely end with a caps with channels=1 and rate=1. https://bugzilla.gnome.org/show_bug.cgi?id=722144 commit bbbd9f7d49ac0635776eba816af08bb533be9ea1 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Mon Jan 13 19:40:49 2014 -0300 tests: audiodecoder: check that negotiation works buffers and gaps Adds 2 tests to verify that output caps are the expected value, reusing input structure values for both buffers and gaps https://bugzilla.gnome.org/show_bug.cgi?id=722144 commit 755414ed1e6994358dc1c6c24983531e23fe9724 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Mon Jan 13 16:33:11 2014 -0300 tests: audiodecoder: add basic playback test for audio decoder Simple test that just check that audio decoding works as expected https://bugzilla.gnome.org/show_bug.cgi?id=722144
I hope we're also reporting the crash to pulseaudio :)
Also pushed to 1.2 commit 03cb702009a9de1a316613e52fe78897774b7653 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Mon Jan 13 20:44:23 2014 -0300 audiodecoder: make sure caps is set before forwarding gap event Before trying to generate a default fixated caps when handling a gap event, make sure that the same strategy that is used when handling a buffer has been attempted. Otherwise audiodecoder will ignore upstream caps settings such as rate and channels and will likely end with a caps with channels=1 and rate=1. https://bugzilla.gnome.org/show_bug.cgi?id=722144 commit 5b4ed671daf2a15104aa7bc469c4451d7cd28ba6 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Mon Jan 13 19:40:49 2014 -0300 tests: audiodecoder: check that negotiation works buffers and gaps Adds 2 tests to verify that output caps are the expected value, reusing input structure values for both buffers and gaps https://bugzilla.gnome.org/show_bug.cgi?id=722144 commit 31c968696ff8c1cc5ec58acc7406132eac7b7ce4 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Mon Jan 13 16:33:11 2014 -0300 tests: audiodecoder: add basic playback test for audio decoder Simple test that just check that audio decoding works as expected https://bugzilla.gnome.org/show_bug.cgi?id=722144
Reopening as another case that leads to this same failure was found.
Created attachment 266272 [details] [review] tests: audiodecoder: add another test for negotiation with gap event Check that even if the subclass doesn't call set_output_format, the base class should use upstream provided caps to fill the output caps that is pushed before the gap event is forwarded, otherwise it ends again fixating the rate and channels to 1.
Created attachment 266273 [details] [review] audiodecoder: copy rate and channels from input before fixating output caps For default caps generation when handling gap events that are sent before any buffer, try to use caps that are closer to what upstream provided to avoid fixating rate or channels to 1 as default
Review of attachment 266273 [details] [review]: Makes sense I guess but for channels>2 you will also need to provide a channel-mask from somewhere. Otherwise looks good and better than using some random configuration.
Review of attachment 266272 [details] [review]: Looks good
Created attachment 266371 [details] [review] audiodecoder: copy rate and channels from input before fixating output caps Updated patch: * Use GST_AUDIO_DEF_CHANNELS/RATE for fixation before doing gst_caps_fixate * Also add channel-mask from either upstream or generated if needed Thanks for the review/help!
Review of attachment 266371 [details] [review]: Some comments still, the loop optimizations obviously apply to all of the loops ::: gst-libs/gst/audio/gstaudiodecoder.c @@ +1871,3 @@ + } + + for (i = 0; i < gst_caps_get_size (caps); i++) { Don't put gst_caps_get_size() in the loop but get the value before it and store it in a variable @@ +1874,3 @@ + gst_structure_fixate_field_nearest_int (gst_caps_get_structure (caps, i), + "channels", GST_AUDIO_DEF_CHANNELS); + gst_structure_fixate_field_nearest_int (gst_caps_get_structure (caps, i), Only get the structure once per iteration and store it in a variable @@ +1880,3 @@ + + /* Need to add a channel-mask if channels > 2 */ + if (channels > 2 You need to get channels from the caps again after fixation, they might be different (if no incaps and DEF_CHANNELS is not supported)
Created attachment 266374 [details] [review] audiodecoder: copy rate and channels from input before fixating output caps Updated patch with the performance fixes and getting the channels again after the _fixate
Review of attachment 266374 [details] [review]: Looks good to me now :)
New patches pushed to master. Thanks for the tips and reviews. commit 36efe20679c85ad72733e55c291df56bc9740f03 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Tue Jan 14 13:02:28 2014 -0300 tests: audiodecoder: add another test for negotiation with gap event Check that even if the subclass doesn't call set_output_format, the base class should use upstream provided caps to fill the output caps that is pushed before the gap event is forwarded, otherwise it ends again fixating the rate and channels to 1. https://bugzilla.gnome.org/show_bug.cgi?id=722144 commit 695ddbd56f95b13e4ed1861bee8531e5079c61ac Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Tue Jan 14 13:05:54 2014 -0300 audiodecoder: copy rate and channels from input before fixating output caps For default caps generation when handling gap events that are sent before any buffer, try to use caps that are closer to what upstream provided to avoid fixating rate or channels to 1 as default. So there are the steps: 1) Try to set rate, channels and channel-mask from upstream if provided 2) Fixate the rate and channels to the default rate and channels from audio lib 3) Fixate the caps just to be sure everything is fixed 4) If no channel-mask was provided and channels > 2, use a default channel-mask (taken from audioconvert code) https://bugzilla.gnome.org/show_bug.cgi?id=722144
Also pushed to 1.2 branch. commit 4ad2ff71dba1d55c544d3b43ece7baa86be6ca0d Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Tue Jan 14 13:02:28 2014 -0300 tests: audiodecoder: add another test for negotiation with gap event commit 6ce882b15150b8b251409ef897513931e7c2a769 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Tue Jan 14 13:05:54 2014 -0300 audiodecoder: copy rate and channels from input before fixating output caps