Closed Bug 785909 Opened 8 years ago Closed 8 years ago

FileMediaResource::Open does blocking IO on the main thread

Categories

(Core :: Audio/Video, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla18
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: bent.mozilla, Assigned: cpearce)

References

Details

(Whiteboard: [Snappy])

Attachments

(1 file, 1 obsolete file)

Found in bug 730765 comment 63, FileMediaResource::Open does blocking IO on the main thread. I can hack around it for now but we should fix it for real.
If this is OOP specific, is this issue B2G specific at this point? Does the main thread IO affect Android or desktop?
This is not B2G specific. The reason it doesn't work with OOP blobs is that those specifically disallow main thread IO. Our normal input streams allow it for now but must not in the future.
Whiteboard: [Snappy]
Yeah, what happens here is that the Available call will in Firefox cause blocking IO, but on B2G will cause the call to fail.

Neither behavior is really acceptable.

It's also a bit unclear why we are doing the Available call at all since for http channels we often simply return -1, i.e. "unknown".
Chris, what side effects does the B2G workaround have? Is it something we can live with for the basecamp release?
blocking-basecamp: + → ?
Whiteboard: [Snappy] → [Snappy][blocked-on-input Chris Pearce]
WebM media in local storage probably won't be impacted, since most WebM files have indexes.

I don't know how MP4 or MP3 files would be affected, Edwin Flores would have a better idea.

Ogg media in local storage won't be able to seek, won't get a duration, and won't report buffered ranges with Bent's workaround however.

I think it would be pretty easy to make this work properly as I outlined in bug 730765 comment 66.

And unfortunately Ogg does I/O on the main thread inside its nsHTMLMediaElement::GetBuffered() handler, so Ogg's going to be broken on B2G anyway. This is bug 737745.
I think libstagefright can play Ogg so we could disable our native Ogg support.
Provided libstagefright's Ogg/Vorbis support is reliable/robust enough, then using that on B2G would  probably be OK for v1.
If it's time critical, a less drastic solution could be to (inside a B2G define) disable nsOggReader's GetBuffered or implement an inaccurate version à la nsMediaPluginReader.cpp.
Comment on attachment 655890 [details] [diff] [review]
Patch

You're missing a lock in FileMediaResource::GetLength (which causes an assertion in EnsureLengthInitialized), but once that is added everything seems to work just fine in my OOP test.

Although, I'd recommend not calling Available() under the lock. That's calling out to unknown code with the mutex held so you risk deadlocks.
(In reply to ben turner [:bent] from comment #10)

Thanks for testing the patch!

> I'd recommend not calling Available() under the lock. That's
> calling out to unknown code with the mutex held so you risk deadlocks.

We need to synchronize access to the file since it can be used from multiple threads. Plus this lock is only used at the bottom of the media call stack, so if there's re-entrancy here it's a bug which I'd like to find!
Attached patch Patch v2Splinter Review
Delay init of FileMediaResource::mSize until we're not on the main thread.

Significantly greener than before:
https://tbpl.mozilla.org/?tree=Try&rev=b46dc56653f5
Assignee: nobody → cpearce
Attachment #655890 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #656319 - Flags: review?(roc)
Whiteboard: [Snappy][blocked-on-input Chris Pearce] → [Snappy]
https://hg.mozilla.org/mozilla-central/rev/e6517dae3b95
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(In reply to Matthew Gregan [:kinetik] from comment #8)
> If it's time critical, a less drastic solution could be to (inside a B2G
> define) disable nsOggReader's GetBuffered or implement an inaccurate version
> à la nsMediaPluginReader.cpp.

Filed bug 786924 to implement this.
Seems like a blocker and even though it's fixed we don't want it to regress.
blocking-basecamp: ? → +
You need to log in before you can comment on or make changes to this bug.