GNOME Bugzilla – Bug 711094
videodecoder: improve max-error handling
Last modified: 2014-12-15 08:58:15 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.
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.
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.
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?
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.
Created attachment 260473 [details] [review] videodecoder: allow using -1 for infinite tolerated errors Updated version with fixes from Tim
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.
commit 81471099d22e7f1d9e2a9b4ac1182b259cc7fb3a commit 0765962fbc963ea9c2c506b4d5b50a80cdcdae5e Thanks for the reviews, pushed.
*** Bug 661747 has been marked as a duplicate of this bug. ***
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.
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 ?
This is now tracked in bug #741542