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 779515 - vorbisdec: Reset decoder in more situations
vorbisdec: Reset decoder in more situations
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other All
: Normal normal
: 1.11.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-03 09:56 UTC by Edward Hervey
Modified: 2017-03-03 10:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vorbisdec: Reset decoder in more situations (1.63 KB, patch)
2017-03-03 09:56 UTC, Edward Hervey
committed Details | Review

Description Edward Hervey 2017-03-03 09:56:20 UTC
See commit message
Comment 1 Edward Hervey 2017-03-03 09:56:26 UTC
Created attachment 347121 [details] [review]
vorbisdec: Reset decoder in more situations

This is a followup commit to b95725c37e70ad3c1ec8dadb401388db375df482

* Resetting the decoder should only happen when we get a new initialization
header (0x01) and not on the other headers
* The initialized variable only gets set to TRUE once all headers have
been parsed. Also check if the vorbis_info struct has been properly resetted
also. Failure to do that would cause vorbisdec to error if it got
two initialization header in a row (the first would configure the underlying
library and the second one would error out because it's already initialized)
Comment 2 Sebastian Dröge (slomo) 2017-03-03 10:12:02 UTC
Review of attachment 347121 [details] [review]:

::: ext/vorbis/gstvorbisdec.c
@@ +587,3 @@
+     * initialized yet. */
+    if ((vd->initialized || (vd->vi.rate != 0)) &&
+        (gst_ogg_packet_data (packet))[0] == 0x01) {

Also the second header with the huffman tables I guess. Just not on comment headers
Comment 3 Edward Hervey 2017-03-03 10:29:33 UTC
Review of attachment 347121 [details] [review]:

::: ext/vorbis/gstvorbisdec.c
@@ +587,3 @@
+     * initialized yet. */
+    if ((vd->initialized || (vd->vi.rate != 0)) &&
+        (gst_ogg_packet_data (packet))[0] == 0x01) {

That would require a more granular system *IF* it's an actual scenario that can happen.

If you reset everything when you get a huffmann table, it will then refuse to decode (it doesn't have the generic information contained in the initialization header).
Comment 4 Sebastian Dröge (slomo) 2017-03-03 10:31:16 UTC
Comment on attachment 347121 [details] [review]
vorbisdec: Reset decoder in more situations

Good point. I'd worry about that then if we ever see such a stream. Go ahead then :)
Comment 5 Edward Hervey 2017-03-03 10:32:14 UTC
commit e575be6dc6689950299049928ad135e456aa07a2
Author: Edward Hervey <edward@centricular.com>
Date:   Fri Mar 3 10:52:15 2017 +0100

    vorbisdec: Reset decoder in more situations
    
    This is a followup commit to b95725c37e70ad3c1ec8dadb401388db375df482
    
    * Resetting the decoder should only happen when we get a new initialization
    header (0x01) and not on the other headers
    * The initialized variable only gets set to TRUE once all headers have
    been parsed. Also check if the vorbis_info struct has been properly resetted
    also. Failure to do that would cause vorbisdec to error if it got
    two initialization header in a row (the first would configure the underlying
    library and the second one would error out because it's already initialized)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=779515