Closed
Bug 874325
Opened 12 years ago
Closed 11 years ago
MediaStreamSource::readAt() is not safe from multi thread access
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)
People
(Reporter: sotaro, Assigned: sotaro)
References
Details
Attachments
(2 files, 1 obsolete file)
1.56 KB,
patch
|
Details | Diff | Splinter Review | |
1.49 KB,
patch
|
Details | Diff | Splinter Review |
MediaStreamSource::readAt() is called from 2 thread. OMXCodec for video and OMXCodec for audio. In the function, following are called. Calling them should be protected from multi thread access, otherwise there might be a case that MediaResource::Read() does not exit. - MediaResource::Tell() - MediaResource::Seek() - MediaResource::Read()
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #752023 -
Flags: review?(chris.double)
Comment 3•12 years ago
|
||
I've had to do the same sort of thing as this in the WMF and DirectShow backends too. At some stage I think we should add a new method to MediaResource ReadAt(offset,length), which uses a lock owned by the MediaResource, so that each backend doesn't need to create a new lock just for this. Does this bug also exist on Android?
Comment 4•12 years ago
|
||
Comment on attachment 752023 [details] [diff] [review] patch - add lock to MediaStreamSource::readAt() Review of attachment 752023 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/OmxDecoder.h @@ +40,5 @@ > // MediaStreamSource is a DataSource that reads from a MPAPI media stream. > class MediaStreamSource : public DataSource { > typedef mozilla::MediaResource MediaResource; > > + Mutex mLock; Add a comment explaining what this lock is for.
Attachment #752023 -
Flags: review?(chris.double) → review+
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #3) > > Does this bug also exist on Android? The code shows same problem exists also on Android. http://mxr.mozilla.org/mozilla-central/source/content/media/plugins/MediaPluginHost.cpp#43
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•12 years ago
|
||
If the patch does not applyed, Youtube video playback could stop because of this defect.
Assignee | ||
Comment 8•11 years ago
|
||
Committable patch. Carry "chris.double: review+"
Attachment #752023 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 11•11 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/0846a72e1761
Keywords: checkin-needed
Comment 12•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0846a72e1761
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 13•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/944f2379f73e https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/944f2379f73e
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-b2g-v1.1hd:
--- → fixed
status-firefox22:
--- → wontfix
status-firefox23:
--- → wontfix
status-firefox24:
--- → fixed
We could probably remove MediaResource::Seek and always pass an offset to Read(). Then the MediaCacheStream's lock would take of this.
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > We could probably remove MediaResource::Seek and always pass an offset to > Read(). Then the MediaCacheStream's lock would take of this. Thanks for the comment. I am going to create a bug for it. But it does not block for leo.
Updated•11 years ago
|
Flags: in-moztrap-
You need to log in
before you can comment on or make changes to this bug.
Description
•