[Bug 779831] New: h264parse (baseparse?) fail to pass not-linked upstream sometimes

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779831

Bug ID: 779831
Summary: h264parse (baseparse?) fail to pass not-linked
upstream sometimes
Classification: Platform
Product: GStreamer
Version: git master
OS: Linux
Status: NEW
Severity: normal
Priority: Normal
Component: gst-plugins-bad
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@noraisin.net
QA Contact: gstreamer-***@lists.freedesktop.org
GNOME version: ---

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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779831

Vincent Penquerc'h <***@collabora.co.uk> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@collabora
| |.co.uk

--- Comment #1 from Vincent Penquerc'h <***@collabora.co.uk> ---
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779831

--- Comment #2 from Vincent Penquerc'h <***@collabora.co.uk> ---
Created attachment 348155
--> https://bugzilla.gnome.org/attachment.cgi?id=348155&action=edit
do not ignore flow errors when dropping data

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779831

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348155|none |reviewed
status| |

--- Comment #3 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 348155:
--> (https://bugzilla.gnome.org/review?bug=779831&attachment=348155)

::: 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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779831

Jan Schmidt <***@noraisin.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@noraisin.net

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779831

--- Comment #4 from Jan Schmidt <***@noraisin.net> ---
Created attachment 348166
--> https://bugzilla.gnome.org/attachment.cgi?id=348166&action=edit
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779831

--- Comment #5 from Jan Schmidt <***@noraisin.net> ---
(In reply to Sebastian Dröge (slomo) from comment #3)

Post by "GStreamer" (GNOME Bugzilla)
::: 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

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779831

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@coaxion.net
Attachment #348166|none |accepted-commit_now
status| |

--- Comment #6 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Comment on attachment 348166
--> https://bugzilla.gnome.org/attachment.cgi?id=348166
baseparse: Don't forget error returns when processing more

That makes more sense to me

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779831

Jan Schmidt <***@noraisin.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348166|accepted-commit_now |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779831

Jan Schmidt <***@noraisin.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
Target Milestone|git master |1.11.3