882567 - gstreamer backend hangs on shutdown if decoder is destroyed before seeking

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Causing mochitest timeouts in content/media/test/test_seek.html; seek10.js.

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Fix

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Green (!) on try:
https://tbpl.mozilla.org/?tree=Try&rev=5dc42fa92b2d

Comment 3

[Alessandro Decina]

Comment on attachment 762441 [details] [diff] [review]
Fix

Review of attachment 762441 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/gstreamer/GStreamerReader.cpp
@@ +362,5 @@
>  
>    *aTags = nullptr;
>  
> +  // Watch the pipeline for fatal errors
> +  gst_bus_add_watch(mBus, GStreamerReader::ErrorCb, this);

gst_bus_add_watch only work if there's a GMainLoop running, which is the case when running under linux, but not in mac. Please use gst_bus_set_sync_handler, which provides the same functionality except the callback is called on pipeline threads instead of being serialized on the main loop. Since the sync handler is called from pipeline threads, you'll need to add locking in the callback (like we do for the preroll and buffer callbacks)

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Fix v2

Addressed comment 3

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Alessandro Decina from comment #3)
> Since the sync handler is called from pipeline threads, you'll
> need to add locking in the callback (like we do for the preroll and buffer
> callbacks)

AFAICT we do all the locking we need to in GStreamerReader::Eos()

Comment 6

[Alessandro Decina]

Comment on attachment 763344 [details] [diff] [review]
Fix v2

Review of attachment 763344 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9173dcf026ee

Comment 8

[(no longer active)]

Backed out because of mochitest-1 timeouts on Linux:

https://hg.mozilla.org/integration/mozilla-inbound/rev/822b350df442

Comment 9

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca547b83df26

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ca547b83df26