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 711094 - videodecoder: improve max-error handling
videodecoder: improve max-error handling
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other All
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 661747 (view as bug list)
Depends on:
Blocks: 710762
 
 
Reported: 2013-10-29 18:15 UTC by Thiago Sousa Santos
Modified: 2014-12-15 08:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
videodecoder: allow using -1 for infinite tolerated errors (1.78 KB, patch)
2013-10-29 18:15 UTC, Thiago Sousa Santos
none Details | Review
videodecoder: error out if no frames are decoded before eos (2.04 KB, patch)
2013-10-29 18:15 UTC, Thiago Sousa Santos
none Details | Review
videodecoder: allow using -1 for infinite tolerated errors (1.78 KB, patch)
2013-11-21 17:30 UTC, Thiago Sousa Santos
committed Details | Review
videodecoder: error out if no frames are decoded before eos (2.60 KB, patch)
2013-11-21 17:30 UTC, Thiago Sousa Santos
committed Details | Review

Description Thiago Sousa Santos 2013-10-29 18:15:17 UTC
Adds 2 new features to max-error handling, the possibility of setting
-1 to tolerate all errors without posting a message.

Also makes videodecoder post an error in case it had errors and received EOS
without having decoded any frames successfully.
Comment 1 Thiago Sousa Santos 2013-10-29 18:15:20 UTC
Created attachment 258474 [details] [review]
videodecoder: allow using -1 for infinite tolerated errors

Allows using -1 to make videodecoder never post an error message
after decoding errors.
Comment 2 Thiago Sousa Santos 2013-10-29 18:15:25 UTC
Created attachment 258475 [details] [review]
videodecoder: error out if no frames are decoded before eos

Raise an error in case no frames are decoded before EOS and there
were errors, meaning that data was received but it was invalid.
Comment 3 Thiago Sousa Santos 2013-10-29 18:17:12 UTC
About the second patch, what is the expected behavior when the stream goes EOS before receiving any data? Is this supposed to post an error too? Even if this is all because of segment clipping? Meaning that valid frames were decoded but falled out of the expected segment.

Does GES have any special requirement for this scenario?
Comment 4 Tim-Philipp Müller 2013-11-20 17:25:50 UTC
Looks fine to me,  but I have three nitpicks:

> +  if (dec->priv->max_errors >= 0 &&
> +      dec->priv->max_errors < dec->priv->error_count) {

While you're changing that section anyway, could you change the second line to

> +      dec->priv->error_count > dec->priv->max_errors) {

? I think that's more natural to read.

And I think it should it read 'was added _in_ 1.4' ?

> +      if (ret && priv->error_count && !priv->pushed_data) {

Please write that as .. && priv->error_count > 0 && ... - it's not a boolean.

(In reply to comment #3)
> About the second patch, what is the expected behavior when the stream goes EOS
> before receiving any data? Is this supposed to post an error too?

I would say no.

> Even if this is all because of segment clipping? Meaning that valid frames
> were decoded but falled out of the expected segment.

In that case definitely not, IMHO.
Comment 5 Thiago Sousa Santos 2013-11-21 17:30:05 UTC
Created attachment 260473 [details] [review]
videodecoder: allow using -1 for infinite tolerated errors

Updated version with fixes from Tim
Comment 6 Thiago Sousa Santos 2013-11-21 17:30:53 UTC
Created attachment 260474 [details] [review]
videodecoder: error out if no frames are decoded before eos

updated patch to always error out when there was input but no output. Even if
buffers are fully clipped, they are considered as valid output.
Comment 7 Thiago Sousa Santos 2013-11-25 13:59:34 UTC
commit 81471099d22e7f1d9e2a9b4ac1182b259cc7fb3a
commit 0765962fbc963ea9c2c506b4d5b50a80cdcdae5e

Thanks for the reviews, pushed.
Comment 8 Sebastian Dröge (slomo) 2013-12-31 08:43:04 UTC
*** Bug 661747 has been marked as a duplicate of this bug. ***
Comment 9 kevin 2014-12-10 09:09:09 UTC
Why need report error when EOS without output? How application handle it? Do application need stop the playback? I think audio can playback normal if video don't report error and just print one warning.
Comment 10 Lyon 2014-12-15 03:04:33 UTC
I have one question:
Could it just send an error print to indicate the corrupt track status instead of sending the ELEMENT_ERROR ?
like  GST_ERROR_OBJECT (dec, "No valid frames decoded before end of stream"); 

Since we found some clips will quit playing in GST1.4 but can continue playing other tracks without corrupt data in GST1.2.

I wonder why should all track stop when there is only one track is corrupt. Since the EOS is send, the corrupt track will stop, but others can continue playing.

And also the same issue in audio track in gstaudiodecoder.c
if just the audio track is broken, the video track can continue playing ,without quit.

Any comment on it ?
Comment 11 Sebastian Dröge (slomo) 2014-12-15 08:58:15 UTC
This is now tracked in bug #741542