[Bug 727767] New: baseparse: Misplaced negative playback rate handling code when pushing frames

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=727767
  GStreamer | gstreamer (core) | git

           Summary: baseparse: Misplaced negative playback rate handling
                    code when pushing frames
    Classification: Platform
           Product: GStreamer
           Version: git
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: Normal
         Component: gstreamer (core)
        AssignedTo: gstreamer-bugs at lists.freedesktop.org
        ReportedBy: vincent.penquerch at collabora.co.uk
         QAContact: gstreamer-bugs at lists.freedesktop.org
     GNOME version: ---


As found by Coverity, some code in baseparse is unreachable:

  ret = gst_base_parse_scan_frame (parse, klass);
  if (ret != GST_FLOW_OK)
    goto done;

  /* eat expected eos signalling past segment in reverse playback */
  if (parse->segment.rate < 0.0 && ret == GST_FLOW_EOS &&
      parse->segment.position >= parse->segment.stop) {
    GST_DEBUG_OBJECT (parse, "downstream has reached end of segment");


Looking back at the history, the ret tested by the EOS condition was not what
_scan_frame returned: there was an intervening call to
gst_base_parse_handle_and_push_frame:

-  ret = gst_base_parse_scan_frame (parse, klass, &frame, TRUE);
+  ret = gst_base_parse_scan_frame (parse, klass, TRUE);
   if (ret != GST_FLOW_OK)
     goto done;

-  /* This always cleans up frame, even if error occurs */
-  ret = gst_base_parse_handle_and_push_frame (parse, klass, &frame);
-
   /* eat expected eos signalling past segment in reverse playback */
   if (parse->segment.rate < 0.0 && ret == GST_FLOW_EOS &&
       parse->segment.position >= parse->segment.stop) {
     GST_DEBUG_OBJECT (parse, "downstream has reached end of segment");

This removal was done in b761f5479a2ca977c07d70be39f1f0eb764b034e. It's not
quite clear to me where this EOS code should be moved. The current
implementation of gst_base_parse_handle_and_push_frame is the only place where
gst_base_parse_handle_and_push_frame is called, but only one of the calls in
the original version had this test/code. Moreover, a comment for
gst_base_parse_finish_frame mentions the return value of
gst_base_parse_handle_and_push_frame should be returned to the caller's caller,
but the negative rate check resets the return if taken, which may have side
effects.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.