Closed Bug 874325 Opened 8 years ago Closed 8 years ago

MediaStreamSource::readAt() is not safe from multi thread access

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

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)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
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)

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()
This is notified by :cpearce.
Blocks: 870564
blocking-b2g: --- → leo?
Attachment #752023 - Flags: review?(chris.double)
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 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+
(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
Status: NEW → ASSIGNED
If the patch does not applyed, Youtube video playback could stop because of this defect.
Blocks a blocker.
blocking-b2g: leo? → leo+
Committable patch. Carry "chris.double: review+"
Attachment #752023 - Attachment is obsolete: true
Committable patch for b2g18. Carry "chris.double: review+"
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0846a72e1761
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
We could probably remove MediaResource::Seek and always pass an offset to Read(). Then the MediaCacheStream's lock would take of this.
(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.
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.