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 779831 - h264parse (baseparse?) fail to pass not-linked upstream sometimes
h264parse (baseparse?) fail to pass not-linked upstream sometimes
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.11.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-10 04:18 UTC by Jan Schmidt
Modified: 2017-03-24 09:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
do not ignore flow errors when dropping data (4.40 KB, patch)
2017-03-17 09:30 UTC, Vincent Penquerc'h
reviewed Details | Review
baseparse: Don't forget error returns when processing more (1.16 KB, patch)
2017-03-17 11:34 UTC, Jan Schmidt
committed Details | Review

Description Jan Schmidt 2017-03-10 04:18:23 UTC
gst-launch-1.0 filesrc location=test-output.ts ! tsdemux ! h264parse ! identity -v

This pipeline should error out with not-linked, but it doesn't. The problem seems to be specific to processing byte-stream data with alignment=nal. With either avc or alignment=au, it will return not-linked as expected.
Comment 1 Vincent Penquerc'h 2017-03-14 10:00:12 UTC
I found a sample which reproduces that, and the bug comes from filler data being marked for dropping by h264parse then dropped by baseparse, which returns GST_FLOW_OK since it doesn't actually push anything, thereby overriding the flow result ultimately returned.

It's not quite clear how that should be fixed. The obvious way would be to keep a "last_ret" field in baseparse, and return this when dropping or otherwise not pushing, instead of OK.
Comment 2 Vincent Penquerc'h 2017-03-17 09:30:03 UTC
Created attachment 348155 [details] [review]
do not ignore flow errors when dropping data
Comment 3 Sebastian Dröge (slomo) 2017-03-17 10:12:47 UTC
Review of attachment 348155 [details] [review]:

::: gst/videoparsers/gsth264parse.c
@@ +1032,3 @@
+      /* Dropping will return GST_FLOW_OK */
+      gst_base_parse_finish_frame (parse, frame, map.size);
+      ret = h264parse->last_ret;

Why do we even get here if non-OK was returned previously? After ERROR was returned, it should have returned that to upstream already.
Comment 4 Jan Schmidt 2017-03-17 11:34:38 UTC
Created attachment 348166 [details] [review]
baseparse: Don't forget error returns when processing more

If parsing returns a non-OK flow return in the middle
of processing an input buffer, don't overwrite that
if a later return is OK again - the subclass might
return not-linked in the middle, and then discard
subsequent data without pushing while returning OK.

A later success doesn't invalidate the earlier failure,
but we should continue processing after not-linked, so
as to keep parse state consistent.
Comment 5 Jan Schmidt 2017-03-17 11:44:04 UTC
(In reply to Sebastian Dröge (slomo) from comment #3)
> Review of attachment 348155 [details] [review] [review]:
> 
> ::: gst/videoparsers/gsth264parse.c
> @@ +1032,3 @@
> +      /* Dropping will return GST_FLOW_OK */
> +      gst_base_parse_finish_frame (parse, frame, map.size);
> +      ret = h264parse->last_ret;
> 
> Why do we even get here if non-OK was returned previously? After ERROR was
> returned, it should have returned that to upstream already.

The problem is baseparse keeps processing more of the input packet on not-linked, and eventually returns whatever the last packet returns
Comment 6 Sebastian Dröge (slomo) 2017-03-17 11:51:40 UTC
Comment on attachment 348166 [details] [review]
baseparse: Don't forget error returns when processing more

That makes more sense to me