Bug 21562 – Layering violation: MediaPlayer should not reference/use FrameView

• Component: Platform
• Status: RESOLVED FIXED

[Sam Weinig]

It is a layering violation for MediaPlayer to non WebCore/platform related classes such as FrameView.

[Carlos Garcia Campos]

Updated bug title, since the MediaPlayer no longer uses FrameView, I think the only layering violation remaining is the CachedResourceLoader.

[Carlos Garcia Campos]

(In reply to comment #1)
> Updated bug title, since the MediaPlayer no longer uses FrameView, I think the only layering violation remaining is the CachedResourceLoader.

Oops, I was wrong, MediaPlayer still uses FrameView. I've been checking it and it's easy to fix for most of the cases. 

 - When checking whether the player has a frame view, we can simply check whether it's visible, because it's equivalent.
 - To check if the player is in a media document, we can add another virtual method to MediaPlayerClient, that HTMLMediaElement can implement just with return document().isMediaDocument()
 - MediaPlayerPrivateQTKit also uses it to get the ColorSpace and Font in order to render the frame rate. I wonder if we could pass those somehow from RenderVideo, or add a MediaPlayer::paintFrameRate(ColorSpace, Font) called after paint() by RenderVideo, and implemented only by MediaPlayerPrivateQTKit. Another approach would be to add media player methods to get the colorspace and font, implemented by the HTMLMediaElement.
 - And finally it's used again by MediaPlayerPrivateQTKit to make the QTMovieView a subview of the document view. I have no idea what this means.

[Eric Carlson]

(In reply to comment #2)
>  - MediaPlayerPrivateQTKit also uses it to get the ColorSpace and Font in order to render the frame rate. I wonder if we could pass those somehow from RenderVideo, or add a MediaPlayer::paintFrameRate(ColorSpace, Font) called after paint() by RenderVideo, and implemented only by MediaPlayerPrivateQTKit. Another approach would be to add media player methods to get the colorspace and font, implemented by the HTMLMediaElement.

Don't worry about that, the frame rate code is not compiled by default and should be deleted in any case. I will remove it when I get some spare time.

>  - And finally it's used again by MediaPlayerPrivateQTKit to make the QTMovieView a subview of the document view. I have no idea what this means.

This code can be removed as well, although it is compiled by default. Why don't you file a bug about removing it and block this bug on it. I will find some time to take care of it in the next couple of days.

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> >  - MediaPlayerPrivateQTKit also uses it to get the ColorSpace and Font in order to render the frame rate. I wonder if we could pass those somehow from RenderVideo, or add a MediaPlayer::paintFrameRate(ColorSpace, Font) called after paint() by RenderVideo, and implemented only by MediaPlayerPrivateQTKit. Another approach would be to add media player methods to get the colorspace and font, implemented by the HTMLMediaElement.
> 
> Don't worry about that, the frame rate code is not compiled by default and should be deleted in any case. I will remove it when I get some spare time.

Oh, much better :-)

> >  - And finally it's used again by MediaPlayerPrivateQTKit to make the QTMovieView a subview of the document view. I have no idea what this means.
> 
> This code can be removed as well, although it is compiled by default. Why don't you file a bug about removing it and block this bug on it. I will find some time to take care of it in the next couple of days.

Sure, thanks for your help.

[Carlos Garcia Campos]

https://bugs.webkit.org/show_bug.cgi?id=137119

Once that one is fixed I'll fix the other cases.

[Carlos Garcia Campos]

Created attachment 239020 [details]
Patch

[Darin Adler]

Comment on attachment 239020 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239020&action=review

Great to do this. I’m concerned about the change in MediaPlayerPrivateAVFoundation::preferredRenderingMode because it’s just removing code. Seems we could have just removed that in a separate patch if it wasn’t needed. I’d like an expert to explain why it’s not needed.

> Source/WebCore/html/HTMLMediaElement.h:586
> +    virtual bool mediaPlayerInMediaDocument() const override;

If it was me I’d probably say const override final, not that it really matters.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:593
> -    if (!m_frameView)
> +    if (!m_mediaPlayerClient)
>          return false;
> -    Document* document = m_frameView->frame().document();
> -    return document && document->isMediaDocument();
> +    return m_visible && m_mediaPlayerClient->mediaPlayerInMediaDocument();

I like writing these as a single line:

    return m_visible && m_mediaPlayerClient && m_mediaPlayerClient->mediaPlayerInMediaDocument();

But also, I wonder if it’s legal for the client to be null. Some other code seems to assume it’s not.

The name m_mediaPlayerClient is so wordy. I really wish it was just m_client given it’s a data member of the MediaPlayer class!

> Source/WebCore/platform/graphics/MediaPlayer.h:267
> +    virtual bool mediaPlayerInMediaDocument() const { return false; }

I’m not so fond of this name, because it sounds like a function that returns “a media player that is in a media document” rather than a function that answers a question about whether the media player is in a media document. I guess maybe the mediaPlayer is a prefix we use on a lot of function names. Maybe mediaPlayerIsInMediaDocument would be better.

But also, maybe there’s a more abstract way to describe this. It seems like a concept layering violation for the media player to even have a concept of “in media document”, so it would be nice to change this concept to instead express what behavior we expect from the media player rather than the situation.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:105
> -    if (!m_player->visible() || !m_player->frameView() || assetStatus() == MediaPlayerAVAssetStatusUnknown)
> +    if (!m_player->visible() || assetStatus() == MediaPlayerAVAssetStatusUnknown)

Why is it OK to take this check out? I don’t see us doing anything new to solve the problem that this check would have been solving. Was it simply an unneeded check?

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:108
>      if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player))

This seems to assume that the client is never null. Was the m_player->frameView() check above perhaps relevant to guaranteeing that? Can the client be null?

[Carlos Garcia Campos]

(In reply to comment #7)
> (From update of attachment 239020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239020&action=review

Thanks for the review.

> Great to do this. I’m concerned about the change in MediaPlayerPrivateAVFoundation::preferredRenderingMode because it’s just removing code. Seems we could have just removed that in a separate patch if it wasn’t needed. I’d like an expert to explain why it’s not needed.

It's removed here, because this patch removes MediaPlayer::frameView().

> > Source/WebCore/html/HTMLMediaElement.h:586
> > +    virtual bool mediaPlayerInMediaDocument() const override;
> 
> If it was me I’d probably say const override final, not that it really matters.
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:593
> > -    if (!m_frameView)
> > +    if (!m_mediaPlayerClient)
> >          return false;
> > -    Document* document = m_frameView->frame().document();
> > -    return document && document->isMediaDocument();
> > +    return m_visible && m_mediaPlayerClient->mediaPlayerInMediaDocument();
> 
> I like writing these as a single line:
> 
>     return m_visible && m_mediaPlayerClient && m_mediaPlayerClient->mediaPlayerInMediaDocument();

I followed the pattern of all other MediaPlayer methods for consistency, that return early if the media player client is null.

> But also, I wonder if it’s legal for the client to be null. Some other code seems to assume it’s not.

I'm not familiar with the media player code, but all the places in MediaPlayer where the client is used, there's a null check first.

> The name m_mediaPlayerClient is so wordy. I really wish it was just m_client given it’s a data member of the MediaPlayer class!

Agree, it also surprised me.

> > Source/WebCore/platform/graphics/MediaPlayer.h:267
> > +    virtual bool mediaPlayerInMediaDocument() const { return false; }
> 
> I’m not so fond of this name, because it sounds like a function that returns “a media player that is in a media document” rather than a function that answers a question about whether the media player is in a media document. I guess maybe the mediaPlayer is a prefix we use on a lot of function names. Maybe mediaPlayerIsInMediaDocument would be better.

Yes, all virtual methods in MediaPlayerClient use the mediaPlayer prefix.

> But also, maybe there’s a more abstract way to describe this. It seems like a concept layering violation for the media player to even have a concept of “in media document”, so it would be nice to change this concept to instead express what behavior we expect from the media player rather than the situation.

I don't really know why it's needed to know whether the media player is in a media document. It's only used by mac.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:105
> > -    if (!m_player->visible() || !m_player->frameView() || assetStatus() == MediaPlayerAVAssetStatusUnknown)
> > +    if (!m_player->visible() || assetStatus() == MediaPlayerAVAssetStatusUnknown)
> 
> Why is it OK to take this check out? I don’t see us doing anything new to solve the problem that this check would have been solving. Was it simply an unneeded check?

Yes, it was unneeded. The only one calling MediaPlayer::setFrameView is RenderVideo, and everytime it was called with a valid frame view, it also called setVisible(true), and when called with null, it also called setVisible(false), so having a valid frame view was equivalent to being visible. Since here we are already checking whether it's visible or not, the frame view check was redundant.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:108
> >      if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player))
> 
> This seems to assume that the client is never null. Was the m_player->frameView() check above perhaps relevant to guaranteeing that? Can the client be null?

Good point, but there are other places in that file where the client is used assuming it's not null and without checking of the player has a frame view, so I'm not sure.

[Darin Adler]

Comment on attachment 239020 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239020&action=review

>>> Source/WebCore/platform/graphics/MediaPlayer.h:267
>>> +    virtual bool mediaPlayerInMediaDocument() const { return false; }
>> 
>> I’m not so fond of this name, because it sounds like a function that returns “a media player that is in a media document” rather than a function that answers a question about whether the media player is in a media document. I guess maybe the mediaPlayer is a prefix we use on a lot of function names. Maybe mediaPlayerIsInMediaDocument would be better.
>> 
>> But also, maybe there’s a more abstract way to describe this. It seems like a concept layering violation for the media player to even have a concept of “in media document”, so it would be nice to change this concept to instead express what behavior we expect from the media player rather than the situation.
> 
> Yes, all virtual methods in MediaPlayerClient use the mediaPlayer prefix.

Please read the rest of my comment, though. That was only the first thing, but the commend said three things. For example, I suggested that mediaPlayerIsInMediaDocument would be a better name.

[Carlos Garcia Campos]

(In reply to comment #9)
> (From update of attachment 239020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239020&action=review
> 
> >>> Source/WebCore/platform/graphics/MediaPlayer.h:267
> >>> +    virtual bool mediaPlayerInMediaDocument() const { return false; }
> >> 
> >> I’m not so fond of this name, because it sounds like a function that returns “a media player that is in a media document” rather than a function that answers a question about whether the media player is in a media document. I guess maybe the mediaPlayer is a prefix we use on a lot of function names. Maybe mediaPlayerIsInMediaDocument would be better.
> >> 
> >> But also, maybe there’s a more abstract way to describe this. It seems like a concept layering violation for the media player to even have a concept of “in media document”, so it would be nice to change this concept to instead express what behavior we expect from the media player rather than the situation.
> > 
> > Yes, all virtual methods in MediaPlayerClient use the mediaPlayer prefix.
> 
> Please read the rest of my comment, though. That was only the first thing, but the commend said three things. For example, I suggested that mediaPlayerIsInMediaDocument would be a better name.

Sure, I read that, but didn't comment anything about the name because I agreed with your suggestions. I'll submit an updated patch anyway.

[Carlos Garcia Campos]

Created attachment 239523 [details]
Patch for landing

[Darin Adler]

(In reply to comment #8)
> (In reply to comment #7)
> > But also, maybe there’s a more abstract way to describe this. It seems like a concept layering violation for the media player to even have a concept of “in media document”, so it would be nice to change this concept to instead express what behavior we expect from the media player rather than the situation.
> 
> I don't really know why it's needed to know whether the media player is in a media document. It's only used by mac.

OK. Maybe some Mac expert can improve this later.

[Carlos Garcia Campos]

Committed r174505: <http://trac.webkit.org/changeset/174505>

[Jer Noble]

(In reply to comment #12)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > But also, maybe there’s a more abstract way to describe this. It seems like a concept layering violation for the media player to even have a concept of “in media document”, so it would be nice to change this concept to instead express what behavior we expect from the media player rather than the situation.
> > 
> > I don't really know why it's needed to know whether the media player is in a media document. It's only used by mac.
> 
> OK. Maybe some Mac expert can improve this later.

This is only used from within our MediaPlayerPrivateQTKit code, so that, when we see media with non-video / non-audio tracks (say, "qtvr" or "rtsp"), we can signal to the MediaDocument that it should fall back to a PluginDocument and use the QuickTime plugin to render that media.

That said, there's no reason why media document knowledge has to be down in QTKit. Instead of only sending a mediaPlayerSawUnsupportedTracks() message from MediaPlayerPrivateQTKit when it's in a media document, it could always send that message, and the HTMLMediaElement could ignore it unless it's in a media document.

[Darin Adler]

(In reply to comment #14)
> That said, there's no reason why media document knowledge has to be down in QTKit. Instead of only sending a mediaPlayerSawUnsupportedTracks() message from MediaPlayerPrivateQTKit when it's in a media document, it could always send that message, and the HTMLMediaElement could ignore it unless it's in a media document.

Sounds like a good thing to do so we can remove mediaPlayerIsInMediaDocument.