Fix the visibility issue of MediaPlayer when painting pixel data from HTMLVideoElement to Canvas

• Component: WebGL
• Status: UNCONFIRMED ---

[Jun Jiang]

In HTMLVideoElement::paintCurrentFrameInContext(), it will call setVisible(true) for the underlying MediaPlayer instance. It may has some historic reason for this since some ports for MediaPlayerPrivate depends on the visibility of MediaPlayer to set-up video Rendering, or depends on the visibility of MediaPlayer to paint. The examples includes MediaPlayerPrivateQuickTimeVisualContext, MediaPlayerPrivateGstreamerBase, MediaPlayerPrivateBlackBerry, etc. 

However, it has side effects and may introduce bugs. For example, in a case, there is a Video element and Canvas element in a web Page and the CSS style for the Video is not set to "display: none". Moreover, to make the video data only shows in the canvas, the visibility of the mediaplayer for the video element is set to false in the JS file. When calling  HTMLVideoElement::paintCurrentFrameInContext() to paint HTMLVideo into canvas, the visibility of mediaplayer changes to true and the video will show in its layout regions, which is not what we want.

It should remove setVisible(true) in HTMLVideoElement::paintCurrentFrameInContext() and re-implement the MediaPlayerPrivate::paintCurrentFrameInContext() for those platforms that depends on the visibility of MediaPlayer to set-up video Rendering, or depends on the visibility of MediaPlayer to paint.

[Kenneth Russell]

Related bug: https://bugs.webkit.org/show_bug.cgi?id=111126

[Jun Jiang]

After some investigation, I found MediaPlayer::setVisible(bool) was intentionally designed to work  in pair with MediaPlayer::paint() and MediaPlayer::paintCurrentFrameInContext(). It is quite different with setting the CSS style to display::None or visibility::hidden. Setting the CSS style display::None or visibility::hidden is always called by JS code and used to to affect the visibility of Video by either not to create the RenderObject or don't paint for the Video in design. Unlike setting display::None and visibility::hidden properties, MediaPlayer::setVisible(bool) controls a internal variable m_visible, which is maintained in MediaPlayer and can not be accessed from JS code and don't affect the Layout and paint of RenderView. So my original assumption that application may change this internal variable m_visible to control video visibility is wrong since this internal variable m_visible can not be accessed by JS code.
Moreover,  MediaPlayer::setVisible(bool) was firstly introduced together with MediaPlayerPrivateQTKit port and was used to setup or tear down some necessary helper objects for video rendering, such as MovieView, MoiveLayer or VideoRender in MediaPlayerPrivateQTKit port. Before MediaPlayer::paint() or MediaPlayer::paintCurrentFrameInContext() was invoked, MediaPlayer::setVisible(true) should be called and used to set up the required objects according to the so called Rendering Mode. When RenderVideo was destructed or MediaPlayer stopped, MediaPlayer::setVisible(false) would be called to tear down the helper objects. 
So for both painting the Video to canvas(MediaPlayer::paintCurrentFrameInContext()) and display the video(MediaPlayer::paint()), MediaPlayer::setVisible(true) is needed. If we don't want to display the video, we can set CSS style to display:None or visibility::hidden but have no way to change the internal variable m_visible in MediaPlayer from JS code. So my assumed failure case won't happen. Back to our original discussion for WebGL case,  MediaPlayer::copyVideoTextureToPlatformTexture() has nothing to do with setting up the helper objects for Video Rendering, therefore, MediaPlayer::setVisible(bool) was not needed.

[Eric Carlson]

(In reply to comment #2)
> After some investigation, I found MediaPlayer::setVisible(bool) was intentionally designed to work  in pair with MediaPlayer::paint() and MediaPlayer::paintCurrentFrameInContext() ...

It sounds like MediaPlayer::setVisible should be renamed.

[Jun Jiang]

Hi, Eric. Thanks for your comments. 
MediaPlayer::setVisible() was first introduced to work with MediaPlayer::paint() to display video in Web Page and acted as a role as mentioned above. It was easy to understand this function and its name. It began a little strange only when the first implementation for MediaPlayer::paintCurrentFrameInContext() was introduced and appeared in the canvas path..
Moreover, as we can see, some platform ports have no implementation for MediaPlayerPrivate::setVisible(bool) and only use MediaPlayer::visible() in MediaPlayer::paint() to check if a previous MediaPlayer::setVisible(true) was called and would return early without true paint otherwise, such as MediaPlayerPrivateGstreamer port. It just follows the interface guidelines to setVisible(true) first, and then a paint(). Therefore, setVisible() interface has two combined meaning now: 1). setup or tear down necessary helper objects. 2). mark up the status of the paint.
Looking further, if MediaPlayer and MediaPlayer::setVisible() are not mean to be exposed to Application via JS bindings in the future, I think current implementation may be already enough compared with adding new APIs to make things much more clear.