Closed Bug 874325 Opened 9 years ago Closed 9 years ago

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


(Firefox OS Graveyard :: General, defect)

Gonk (Firefox OS)
Not set


(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)

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


(Reporter: sotaro, Assigned: sotaro)




(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.
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
Closed: 9 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.