baseparse: Misplaced negative playback rate handling code when pushing frames
@vincent
Submitted by Vincent Penquerc'h Link to original bug (#727767)
Description
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 b761f547. 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.