GNOME Bugzilla – Bug 748439
vtdec: strange error handling
Last modified: 2018-11-03 13:34:32 UTC
In gst_vtdec_handle_frame: status = VTDecompressionSessionDecodeFrame (vtdec->session, cm_sample_buffer, input_flags, frame, NULL); if (status != noErr && FALSE) goto error; I guess the "&& FALSE" is debug-time code that sneaked into the commit. Introduced in: commit b1a756fda730d5edde0d6d83df723d8923008f98 Author: Alessandro Decina <alessandro.d@gmail.com> Date: Sat Dec 7 23:55:13 2013 +0100 applemedia: rewrite VideoToolbox decoder based on GstVideoDecoder
Yes, it should also probably no go into a GST_ELEMENT_ERROR() but instead GST_VIDEO_DECODER_ERROR() (they're basically the same, just that the latter only is a fatal error if there are a few consecutive errors without producing a new frame)
Can you check if doing that breaks something?
Created attachment 302442 [details] [review] vtdec: use GST_VIDEO_DECODER_ERROR This patch works. For normal decoding, everything is fine. For vtenc_h264 ! breakmydata probability=0.1 ! vtdec, we get a series of errors and bail out after GST_VIDEO_DECODER_MAX_ERRORS (= 10). What troubles me is that 'gst_video_decoder_set_max_errors' is not called anywhere (and in particular, not configurable via props). e.g. for an RTC use-case, do we really want anything other than -1 ? Also, to keep compatibility, I'd default vtdec's max_errors to -1.
Whith breakmydata probability=0.1, the probably of having a single non-corrupted frame is quite low. Breakmysdata works per bytes. In real life, you are more likely to have lost packet then corrupted packets (see identity drop-probability=).
(In reply to Nicolas Dufresne (stormer) from comment #4) > In real life, you are more likely to have lost packet then corrupted packets (see > identity drop-probability=). This seems true, e.g. for udpsrc. While I think it'll be good for GstVideoDecoder-based elements to offer a max_errors property, I think it's out of scope for this patch. If it's good, please merge.
@stormer ping :)
Review of attachment 302442 [details] [review]: ::: sys/applemedia/vtdec.c @@ -383,3 @@ VTDecompressionSessionDecodeFrame (vtdec->session, cm_sample_buffer, input_flags, frame, NULL); - if (status != noErr && FALSE) Indeed, that was strange, I wonder who reviewed Alessandro's patches ... @@ +395,3 @@ error: + GST_VIDEO_DECODER_ERROR (vtdec, 1, STREAM, DECODE, (NULL), + ("VTDecompressionSessionDecodeFrame returned %d", (int) status), ret); Don't we need to also call gst_video_decoder_drop_frame(), otherwise if this is not the maximum error count, we'll endup leaking this frame in the decoder frame list no ? (I might be missing context with just the diff here, let me know).
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/243.