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 794192 - videodecoder: Event order may get mixed up
videodecoder: Event order may get mixed up
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.14.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-03-09 07:57 UTC by matthias.fend
Modified: 2018-05-01 08:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
videodecoder-keep-event-order.patch (1.78 KB, patch)
2018-03-09 07:57 UTC, matthias.fend
committed Details | Review
videodecoder: add test for event order (4.36 KB, patch)
2018-04-20 10:35 UTC, Michael Olbrich
committed Details | Review

Description matthias.fend 2018-03-09 07:57:53 UTC
Created attachment 369480 [details] [review]
videodecoder-keep-event-order.patch

If the videodecoder has multiple frames to push out it may happen that the queued events are pushed out in wrong order.
This issue was observed with image-orientation tag events. A broken event order my result in a final invalid rotation.
Comment 1 Sebastian Dröge (slomo) 2018-03-09 14:23:25 UTC
Comment on attachment 369480 [details] [review]
videodecoder-keep-event-order.patch

This seems correct, but a test would be nice.

I'm not sure if the loop in gst_video_decoder_flush_parse() also needs to be changed to use g_list_prepend() instead of _append(), can you check?


The loop for pushing out all events walks from the last to the first event, so newer events have to be added to the beginning.
Comment 2 matthias.fend 2018-03-12 14:31:37 UTC
Regarding gst_video_decoder_flush_parse() I'm also not really sure. 
I recognized this issue when feeding live data via an appsrc into the pipeline (additionally the occurrence was strongly dependent on timing).
Since gst_video_decoder_flush_parse() seems to be only used for reverse playback I can't really test it with my setup.
Maybe you have an idea how to test and verify it?
Comment 3 Michael Olbrich 2018-04-19 14:23:06 UTC
About gst_video_decoder_flush_parse(): I'm not sure if this should be changed:
Only events '<= segment' are considered. 'flush-stop' and 'stream-start' are forwarded immediately, and caps events are consumed. So in reality this loop only handles segment events.
I'm not sure if we can have more than one segment event here. I don't think that would work correctly.
Comment 4 Michael Olbrich 2018-04-20 10:35:39 UTC
Created attachment 371153 [details] [review]
videodecoder: add test for event order

When frames are dropped or reordered then the serialized events are
collected and pushed with the next frame. This test verifies that the
order is preserved.
Comment 5 Nicolas Dufresne (ndufresne) 2018-04-26 21:05:51 UTC
Attachment 371153 [details] pushed as 1b36477 - videodecoder: add test for event order
Attachment 369480 [details] pushed as 41c6efb - videodecoder-keep-event-order.patch