Closed Bug 973379 Opened 6 years ago Closed 6 years ago

GStreamerReader::ReadMetadata()'s element config loop should break on error to avoid timeout

Categories

(Core :: Audio/Video, defect)

29 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: cpearce, Assigned: alessandro.d)

References

Details

Attachments

(1 file, 1 obsolete file)

The "for (i in G_N_ELEMENTS(flags))" loop in GStreamerReader::ReadMetadata() is not breaking out of the loop when gst_bus_timed_pop_filtered() is popping an error message. This means if we receive an error message on the first iteration through the loop, we'll set the gst pipeline to null state, which flushes the gst_bus, causing it to drop all further messages. This means on subsequent iterations through the loop we'll block in gst_bus_timed_pop_filtered() again waiting for a message that will never come, since we've flushed the gst_bus; the bus will drop all events posted to it, without waking up the thread blocked in gst_bus_timed_pop_filtered(). So we'll get a timeout in gst_bus_timed_pop_filtered().

This causes mochitest timeouts with some of the patches I'm working on applied:

8600 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_bug448534.html | Test timed out.
8605 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_bug463162.xhtml | Test timed out.
8611 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_bug495145.html | Test timed out.
8617 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_bug495300.html | Test timed out.
8618 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | 4 test timeouts, giving up.

... which I can reproduce locally in my Ubuntu 12.04 VM with the following command:

./mach mochitest-plain --start-at content/media/test/test_autoplay_contentEditable.html --end-at content/media/test/test_bug495300.html --run-until-failure --repeat 10000

We're hitting this condition because we're loading a file and shutting it down before the GStreamerReader has had a chance to initialize (in test_autoplay_contentEditable I think).
Blocks: 968016
Summary: GStreamerReader::ReadMetada()'s element config loop should break on error to avoid timeout → GStreamerReader::ReadMetadata()'s element config loop should break on error to avoid timeout
Blocks: 961197
Blocks: 957908
Attached patch bug973379.patchSplinter Review
After debugging the bug with Chris on IRC, we came up with this patch.

The core of the issue was that we were not checking the return code of gst_element_set_state(mPlayBin, GST_STATE_PAUSED) in ReadMetadata. When that calls returns FAILURE (because the stream is closed before the pipeline is started for example), no ERROR message is posted on the bus, so the gst_bus_timed_pop_filtered(bus, ..., GST_MESSAGE_ERROR) call that follows keeps waiting indefinitely for an error that will never arrive.
Attachment #8376872 - Attachment is obsolete: true
Attachment #8376872 - Flags: review?(edwin)
Attachment #8376949 - Flags: review?(edwin)
Comment on attachment 8376949 [details] [diff] [review]
bug973379.patch

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

Ouch. Nice catch.

::: content/media/gstreamer/GStreamerReader.cpp
@@ +394,3 @@
>        gst_element_set_state(mPlayBin, GST_STATE_NULL);
> +      if (message)
> +        gst_message_unref(message);

Braces! :)
Attachment #8376949 - Flags: review?(edwin) → review+
I addressed edwin's comment, and landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/047ac8e24409
Assignee: cpearce → alessandro.d
(In reply to Edwin Flores [:eflores] [:edwin] from comment #3)
> Comment on attachment 8376949 [details] [diff] [review]
> bug973379.patch
> 
> Review of attachment 8376949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ouch. Nice catch.
> 
> ::: content/media/gstreamer/GStreamerReader.cpp
> @@ +394,3 @@
> >        gst_element_set_state(mPlayBin, GST_STATE_NULL);
> > +      if (message)
> > +        gst_message_unref(message);
> 
> Braces! :)

One day... one day!
https://hg.mozilla.org/mozilla-central/rev/047ac8e24409
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.