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 666579 - [a52dec] behavior changed since porting to audiodecoder
[a52dec] behavior changed since porting to audiodecoder
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-ugly
git master
Other All
: Normal normal
: 0.10.20
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-12-20 11:06 UTC by Julien Isorce
Modified: 2011-12-20 12:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
AC3 sample to reproduce the problem (357.75 KB, audio/ac3)
2011-12-20 11:06 UTC, Julien Isorce
  Details
check that a52_init return a valid a52 state (934 bytes, patch)
2011-12-20 11:08 UTC, Julien Isorce
none Details | Review
a52dec: check that a52_init returns a valid state (934 bytes, patch)
2011-12-20 11:45 UTC, Mark Nauwelaerts
committed Details | Review

Description Julien Isorce 2011-12-20 11:06:12 UTC
Created attachment 203938 [details]
AC3 sample to reproduce the problem

Thanks for porting a52dec to audioencoder. But it seems that the behavior changed about "discont" buffers.

 * Steps to reproduce:

gst-launch-0.10 filesrc location=sample.ac3 ! ac3parse ! a52dec ! fakesink

 * Actual result:

GstPipeline:pipeline0/GstA52Dec:a52dec0: Could not decode stream

 * Previsous result:

No error

 * Other informations:

The check around "if (a52_block (a52dec->state))" and "if (a52_frame)" is now a GST_AUDIO_DECODER_ERROR. Previously it was a GST_WARNING with a discont=TRUE.
Also the line "GST_BUFFER_FLAG_SET (buf, GST_BUFFER_FLAG_DISCONT);" disappeared.
Comment 1 Julien Isorce 2011-12-20 11:08:59 UTC
Created attachment 203940 [details] [review]
check that a52_init return a valid a52 state

This check has been deleted when porting to audioencoder
Comment 2 Mark Nauwelaerts 2011-12-20 11:45:43 UTC
Created attachment 203943 [details] [review]
a52dec: check that a52_init returns a valid state

Same as previous patch, but with a minor nitpick on the error code adjusted.
Comment 3 Mark Nauwelaerts 2011-12-20 11:53:54 UTC
Thanks, committed:

commit 24fca6cde5bde1ff679433c0744add20255bb04a
Author: Julien Isorce <julien.isorce@gmail.com>
Date:   Tue Dec 20 11:54:38 2011 +0100

    a52dec: check that a52_init returns a valid a52 state
Comment 4 Mark Nauwelaerts 2011-12-20 12:01:22 UTC
As for the remaining behaviour; the old one was not entirely nice/ok, as it would totally ignore errors and possibly carry on indefinitely.  Evidently, the new is not quite convenient either giving up at the first sign of trouble.

Following minor -base commit to audiodecoder arranges for intended behaviour, which is to only really give up in seriously problematic cases (ongoing errors).  The remaining discont marking/handling is all done automagically by the specialized GST_AUDIO_DECODER_ERROR macro and the rest of buffer handling code.

----

commit c41f3cbef03417c3025b145122788502764313ad
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Tue Dec 20 12:42:18 2011 +0100

    audiodecoder: set a non-zero default maximum tolerated errors
    
    Whereas the previous default 0 was backwards compatible in that it lead
    to erroring out immediately upon any error, elements that are really
    ported and using the base class error macro can be assumed to intend to
    improve behaviour rather than maintaining the old one.  So, make it easy
    on those and any future one and tolerate some errors by default, as intended.
    
    Fixes #666579.

... and in 0.10 release branch ...

commit 10aabf623abfa17b29a1e65d403923e36716c3a7
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Tue Dec 20 12:42:18 2011 +0100

    audiodecoder: set a non-zero default maximum tolerated errors
    
    Whereas the previous default 0 was backwards compatible in that it lead
    to erroring out immediately upon any error, elements that are really
    ported and using the base class error macro can be assumed to intend to
    improve behaviour rather than maintaining the old one.  So, make it easy
    on those and any future one and tolerate some errors by default, as intended.
    
    Fixes #666579.