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 722144 - audiodecoder: do not negotiate caps with rate=1 and channels=1 for gap
audiodecoder: do not negotiate caps with rate=1 and channels=1 for gap
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other All
: Normal normal
: 1.2.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-01-14 00:02 UTC by Thiago Sousa Santos
Modified: 2014-01-15 18:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tests: audiodecoder: add basic playback test for audio decoder (9.75 KB, patch)
2014-01-14 00:02 UTC, Thiago Sousa Santos
committed Details | Review
tests: audiodecoder: check that negotiation works buffers and gaps (3.88 KB, patch)
2014-01-14 00:02 UTC, Thiago Sousa Santos
committed Details | Review
audiodecoder: make sure caps is set before forwarding gap event (5.40 KB, patch)
2014-01-14 00:02 UTC, Thiago Sousa Santos
committed Details | Review
tests: audiodecoder: add another test for negotiation with gap event (4.23 KB, patch)
2014-01-14 16:10 UTC, Thiago Sousa Santos
committed Details | Review
audiodecoder: copy rate and channels from input before fixating output caps (2.13 KB, patch)
2014-01-14 16:10 UTC, Thiago Sousa Santos
reviewed Details | Review
audiodecoder: copy rate and channels from input before fixating output caps (6.11 KB, patch)
2014-01-15 17:11 UTC, Thiago Sousa Santos
needs-work Details | Review
audiodecoder: copy rate and channels from input before fixating output caps (6.06 KB, patch)
2014-01-15 17:34 UTC, Thiago Sousa Santos
committed Details | Review

Description Thiago Sousa Santos 2014-01-14 00:02:19 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.
Comment 1 Thiago Sousa Santos 2014-01-14 00:02:23 UTC
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
Comment 2 Thiago Sousa Santos 2014-01-14 00:02:32 UTC
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
Comment 3 Thiago Sousa Santos 2014-01-14 00:02:36 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2014-01-14 10:25:05 UTC
Review of attachment 266216 [details] [review]:

Looks good, good work :)
Comment 5 Thiago Sousa Santos 2014-01-14 12:46:55 UTC
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
Comment 6 Jan Schmidt 2014-01-14 13:02:25 UTC
I hope we're also reporting the crash to pulseaudio :)
Comment 7 Thiago Sousa Santos 2014-01-14 13:32:15 UTC
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
Comment 8 Thiago Sousa Santos 2014-01-14 16:03:27 UTC
Reopening as another case that leads to this same failure was found.
Comment 9 Thiago Sousa Santos 2014-01-14 16:10:01 UTC
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.
Comment 10 Thiago Sousa Santos 2014-01-14 16:10:06 UTC
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
Comment 11 Sebastian Dröge (slomo) 2014-01-15 15:32:44 UTC
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.
Comment 12 Sebastian Dröge (slomo) 2014-01-15 15:33:48 UTC
Review of attachment 266272 [details] [review]:

Looks good
Comment 13 Thiago Sousa Santos 2014-01-15 17:11:22 UTC
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!
Comment 14 Sebastian Dröge (slomo) 2014-01-15 17:16:53 UTC
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)
Comment 15 Thiago Sousa Santos 2014-01-15 17:34:56 UTC
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
Comment 16 Sebastian Dröge (slomo) 2014-01-15 17:41:14 UTC
Review of attachment 266374 [details] [review]:

Looks good to me now :)
Comment 17 Thiago Sousa Santos 2014-01-15 18:26:46 UTC
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
Comment 18 Thiago Sousa Santos 2014-01-15 18:33:18 UTC
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