Closed
Bug 785909
Opened 12 years ago
Closed 12 years ago
FileMediaResource::Open does blocking IO on the main thread
Categories
(Core :: Audio/Video, defect, P1)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: bent.mozilla, Assigned: cpearce)
References
Details
(Whiteboard: [Snappy])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•12 years ago
|
||
If this is OOP specific, is this issue B2G specific at this point? Does the main thread IO affect Android or desktop?
Reporter | ||
Comment 2•12 years ago
|
||
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.
Updated•12 years ago
|
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]
Assignee | ||
Comment 5•12 years ago
|
||
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•12 years ago
|
||
I think libstagefright can play Ogg so we could disable our native Ogg support.
Assignee | ||
Comment 7•12 years ago
|
||
Provided libstagefright's Ogg/Vorbis support is reliable/robust enough, then using that on B2G would probably be OK for v1.
Comment 8•12 years ago
|
||
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.
Assignee | ||
Comment 9•12 years ago
|
||
The fix I mentioned in bug 730765 comment 66.
https://tbpl.mozilla.org/?tree=Try&rev=9e368e55bad7
Reporter | ||
Comment 10•12 years ago
|
||
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.
Assignee | ||
Comment 11•12 years ago
|
||
(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!
Assignee | ||
Comment 12•12 years ago
|
||
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•12 years ago
|
Whiteboard: [Snappy][blocked-on-input Chris Pearce] → [Snappy]
Attachment #656319 -
Flags: review?(roc) → review+
Assignee | ||
Comment 13•12 years ago
|
||
Comment 14•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Assignee | ||
Comment 15•12 years ago
|
||
(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.
Comment 16•12 years ago
|
||
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.
Description
•