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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Sotaro Ikeda [:sotaro]]

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

Comment 1

[Sotaro Ikeda [:sotaro]]

This is notified by :cpearce.

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - add lock to MediaStreamSource::readAt()

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

[cajbir (:cajbir)]

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.

Comment 5

[Sotaro Ikeda [:sotaro]]

(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

Comment 6

[Sotaro Ikeda [:sotaro]]

If the patch does not applyed, Youtube video playback could stop because of this defect.

Comment 7

[Alex Keybl [:akeybl]]

Blocks a blocker.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - add lock to MediaStreamSource::readAt()

Committable patch. Carry "chris.double: review+"

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 for b2g18 - add lock to MediaStreamSource::readAt()

Committable patch for b2g18. Carry "chris.double: review+"

Comment 10

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=f25b2c699c05

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/0846a72e1761

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0846a72e1761

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/944f2379f73e
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/944f2379f73e

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could probably remove MediaResource::Seek and always pass an offset to Read(). Then the MediaCacheStream's lock would take of this.

Comment 15

[Sotaro Ikeda [:sotaro]]

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