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 748439 - vtdec: strange error handling
vtdec: strange error handling
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: git master
Assigned To: Ilya Konstantinov
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-04-25 04:23 UTC by Ilya Konstantinov
Modified: 2018-11-03 13:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vtdec: use GST_VIDEO_DECODER_ERROR (1.17 KB, patch)
2015-04-27 13:47 UTC, Ilya Konstantinov
reviewed Details | Review

Description Ilya Konstantinov 2015-04-25 04:23:47 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
Comment 1 Sebastian Dröge (slomo) 2015-04-26 17:30:23 UTC
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)
Comment 2 Sebastian Dröge (slomo) 2015-04-26 17:30:39 UTC
Can you check if doing that breaks something?
Comment 3 Ilya Konstantinov 2015-04-27 13:47:53 UTC
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.
Comment 4 Nicolas Dufresne (ndufresne) 2015-04-27 14:11:18 UTC
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=).
Comment 5 Ilya Konstantinov 2015-04-28 13:02:08 UTC
(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.
Comment 6 Ilya Konstantinov 2015-07-26 11:45:25 UTC
@stormer ping :)
Comment 7 Nicolas Dufresne (ndufresne) 2015-07-26 22:14:45 UTC
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).
Comment 8 GStreamer system administrator 2018-11-03 13:34:32 UTC
-- 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.