FileMediaResource::Open does blocking IO on the main thread

RESOLVED FIXED in mozilla18

Status

()

defect
P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bent.mozilla, Assigned: cpearce)

Tracking

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment, 1 obsolete attachment)

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.

Comment 6

7 years ago
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!
Posted 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)
Assignee

Updated

7 years ago
Whiteboard: [Snappy][blocked-on-input Chris Pearce] → [Snappy]
https://hg.mozilla.org/mozilla-central/rev/e6517dae3b95
Status: ASSIGNED → RESOLVED
Closed: 7 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.