Closed Bug 1347402 Opened 7 years ago Closed 7 years ago

Dont' use MediaDecoderOwner::GetMediaElement() in MediaDecoder

Categories

(Core :: Audio/Video: Playback, enhancement)

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1347402 part 1 - call DownloadSuspended() via polymorphism;
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1347402 part 2 - open a GetOwnerDoc() interface at the MediaDecoderOwner;
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1347402 part 3 - get owner document via polymorphism;
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1347402 part 4 - move ConstructMediaTracks/RemoveMediaTracks to HTMLMediaElemnt;
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1347402 part 5 - create MediaElementGMPCrashHelper in HTMLMediaElement.cpp;
59 bytes, text/x-review-board-request
cpearce
: review+
jwwang
: review+
Details 
Somehow, the MeidaDecoder should not have logic that depends on its owner's type.
Assignee: nobody → kaku
Blocks: 1347406
Status: NEW → ASSIGNED
@Chirs, 

MeidaDecoder might instantiate a MediaElementGMPCrashHelper which keeps a weak reference to an HTMLMediaElement (http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/dom/media/MediaDecoder.cpp#975-994). 

I wonder if we could modify the MediaElementGMPCrashHelper to be the same to the BufferDecoderGMPCrashHelper which holds a weak reference to a nsPIDOMWindowInner directly (http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/dom/media/webaudio/MediaBufferDecoder.cpp#167-184)?

By this way, we can remove the dependency to HTMLMediaElement from MediaDecoder. I think the change should be okay, but I'm not quite sure because you added these two classes in bug 1267918 with different implementations and I wonder if there are any tricks?
Flags: needinfo?(cpearce)
If the media element moves between windows (which can happen if script appends the media element into a same-origin iframe's document or a new window's document), the GMPCrashHelper needs to be able to find the new window in the case of a GMP crash so that the crash prompt appears in the window where the GMP is running.

So you can't assume that the window in which a media element is at time of creation is the window in which it is at the time of a crash.

I can't remember why I thought what the BufferDecoderGMPCrashHelper is doing is OK. Regardless, we don't support unencrypted decoding via GMP anymore, so the BufferDecoderGMPCrashHelper can now be removed.

If you want to test this, you can crash a GMP by toggling the pref media.gmp.plugin.crash to true.

You can test EME on https://people-mozilla.org/~cpearce/mse-clearkey/
Flags: needinfo?(cpearce)
Got it, then I think we can at least move the creation of MediaElementGMPCrashHelper.cpp to HTMLMediaElement, and let MediaDecoder get one via the MediaDecoderOwner. Thanks!
Type:
...... move the creation of MediaElementGMPCrashHelper to HTMLMediaElement.cpp, ......
Comment on attachment 8847924 [details]
Bug 1347402 part 1 - call DownloadSuspended() via polymorphism;

https://reviewboard.mozilla.org/r/120852/#review122856

::: dom/media/MediaDecoder.cpp:255
(Diff revision 2)
>      if (!self->mDecoder) {
>        return;
>      }
>      self->mDecoder->NotifyDownloadEnded(aStatus);
>      if (NS_SUCCEEDED(aStatus)) {
> -      HTMLMediaElement* element = self->GetMediaOwner()->GetMediaElement();
> +      MediaDecoderOwner* owner = self->GetMediaOwner();

The ogiginal code suggests owner can't be null.
Attachment #8847924 - Flags: review?(jwwang) → review+
Comment on attachment 8847925 [details]
Bug 1347402 part 2 - open a GetOwnerDoc() interface at the MediaDecoderOwner;

https://reviewboard.mozilla.org/r/120854/#review122860

::: commit-message-fb5ca:3
(Diff revision 2)
> +Bug 1347402 part 2 - open a GetOwnerDoc() interface at the MediaDecoderOwner; r?jwwang
> +
> +Open a GetOwnerDoc() interface at the MediaDecoderOwner and then we can get the

Add a Blah() function to the XXX interface.

::: dom/html/HTMLMediaElement.h:777
(Diff revision 2)
>      CREATE_IMAGEBITMAP,
>      CAPTURE_STREAM,
>    };
>    void MarkAsContentSource(CallerAPI aAPI);
>  
> +  nsIDocument *GetOwnerDoc() const override;

style: |Foo*|.

::: dom/html/HTMLMediaElement.cpp:7412
(Diff revision 2)
>                                           mVisibilityState == Visibility::APPROXIMATELY_VISIBLE,
>                                           IsInUncomposedDoc());
>    }
>  }
>  
> +nsIDocument *

style:
|Foo*| instead of |Foo *|.

::: dom/media/MediaDecoderOwner.h:161
(Diff revision 2)
>    // provided init data. Actual dispatch may be delayed until HAVE_METADATA.
>    // Main thread only.
>    virtual void DispatchEncrypted(const nsTArray<uint8_t>& aInitData,
>                                   const nsAString& aInitDataType) = 0;
> +
> +  virtual nsIDocument *GetOwnerDoc() const = 0;

Be sure to also modify MockMediaDecoderOwner. Also add comments to describe the purpose of this function.

style:
Foo* GetFoo();
Attachment #8847925 - Flags: review?(jwwang) → review+
Comment on attachment 8847926 [details]
Bug 1347402 part 3 - get owner document via polymorphism;

https://reviewboard.mozilla.org/r/120856/#review122862

::: dom/media/MediaDecoder.cpp:592
(Diff revision 2)
>  MediaDecoder::OnDecoderDoctorEvent(DecoderDoctorEvent aEvent)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    // OnDecoderDoctorEvent is disconnected at shutdown time.
>    MOZ_DIAGNOSTIC_ASSERT(!IsShutdown());
> -  HTMLMediaElement* element = GetOwner()->GetMediaElement();
> +  nsIDocument* doc = GetOwner()->GetOwnerDoc();

The function name is confusing.
How about: GetOwner()->GetDocument() for the 2 owners mean different things.

The 'Owner' in GetOwner refers to the owner which own this MediaDecoder. While the 'Owner' in GetOwnerDoc refers to the owner which own the MediaDecoderOwner.

::: dom/media/MediaDecoder.cpp:1296
(Diff revision 2)
>  MediaDecoder::NotifyCompositor()
>  {
>    MediaDecoderOwner* owner = GetOwner();
>    NS_ENSURE_TRUE_VOID(owner);
>  
> -  dom::HTMLMediaElement* element = owner->GetMediaElement();
> +  nsIDocument *ownerDoc = owner->GetOwnerDoc();

style: Foo*.
Attachment #8847926 - Flags: review?(jwwang) → review+
Comment on attachment 8847927 [details]
Bug 1347402 part 4 - move ConstructMediaTracks/RemoveMediaTracks to HTMLMediaElemnt;

https://reviewboard.mozilla.org/r/120858/#review122866

::: dom/media/MediaDecoderOwner.h:166
(Diff revision 2)
>  
>    virtual nsIDocument *GetOwnerDoc() const = 0;
> +
> +  // Called by the media decoder to create audio/video tracks and add to its
> +  // owner's track list.
> +  virtual void ConstructMediaTracks(const MediaInfo* aInfo) = 0;

Don't foreget MockMediaDecoderOwner.
Attachment #8847927 - Flags: review?(jwwang) → review+
Comment on attachment 8847928 [details]
Bug 1347402 part 5 - create MediaElementGMPCrashHelper in HTMLMediaElement.cpp;

https://reviewboard.mozilla.org/r/120860/#review122870

::: dom/media/MediaDecoderOwner.h:174
(Diff revision 2)
>    // Called by the media decoder to removes all audio/video tracks from its
>    // owner's track list.
>    virtual void RemoveMediaTracks() = 0;
> +
> +  // Called by the media decoder to create a GMPCrashHelper.
> +  virtual already_AddRefed<GMPCrashHelper> CreateGMPCrashHelper() = 0;

Don't forget MockMediaDecoderOwner.
Attachment #8847928 - Flags: review?(jwwang) → review+
Comment on attachment 8847928 [details]
Bug 1347402 part 5 - create MediaElementGMPCrashHelper in HTMLMediaElement.cpp;

https://reviewboard.mozilla.org/r/120860/#review123274
Attachment #8847928 - Flags: review?(cpearce) → review+
Comment on attachment 8847925 [details]
Bug 1347402 part 2 - open a GetOwnerDoc() interface at the MediaDecoderOwner;

https://reviewboard.mozilla.org/r/120854/#review122860

> Be sure to also modify MockMediaDecoderOwner. Also add comments to describe the purpose of this function.
> 
> style:
> Foo* GetFoo();

Will make the MockMediaDecoderOwner return a nullptr.
Try looks good, thanks for the review!
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1d1f787c988
part 1 - call DownloadSuspended() via polymorphism; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/6307b8e856ac
part 2 - open a GetOwnerDoc() interface at the MediaDecoderOwner; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/421c83786af7
part 3 - get owner document via polymorphism; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/3ad2b6cfdc29
part 4 - move ConstructMediaTracks/RemoveMediaTracks to HTMLMediaElemnt; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/23cfb067130f
part 5 - create MediaElementGMPCrashHelper in HTMLMediaElement.cpp; r=cpearce,jwwang
Keywords: checkin-needed
Depends on: 1348432
Depends on: 1360123
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: