Bug 38689 – #34005 will break fullscreen video playback

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jer Noble]

The changes for bug https://bugs.webkit.org/show_bug.cgi?id=34005 will cause fullscreen support for video playback on windows to either a) not work or b) crash.  This is because FullscreenVideoController reinterpret_casts the current PlatformMedia returned by MediaPlayer to the incorrect type, and methods called on that pointer fail.

The fix is to rewrite the PlatformMedia struct so that it indicates which type is correct, and to have the FullscreenVideoController check that type before calling functions on the returned pointer.

[Jer Noble]

Created attachment 55297 [details]
Part 1: PlatformMedia redefinition

This is the new proposed definition of PlatformMedia.  Instead of the definition of PlatformMedia being determined at compile-time, the definition can be chosen at runtime, allowing multiple MediaPlayerPrivate implementations to return different PlatformMedia within the same runtime.  It also helps to eliminate the possibility of casting to the wrong type.

[Eric Seidel (no email)]

Comment on attachment 55297 [details]
Part 1: PlatformMedia redefinition

Hmmm.  I guess that's one way.  Another would be a wrapper object which knew out to vend the various information from these things when needed and could do the various lifetime management for each.  Said wrapper object could then be held in an OwnPtr or RefPtr as necessary...

[WebKit Review Bot]

Attachment 55297 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2238076

[Jer Noble]

Created attachment 55552 [details]
Redefinition of PlatformMedia

This patch supersedes the previous patch, and includes all the changes to PlatformMedia clients to match the new definition.

[WebKit Review Bot]

Attachment 55552 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:605:  Line contains tab character.  [whitespace/tab] [5]
WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:606:  Line contains tab character.  [whitespace/tab] [5]
WebKit/win/FullscreenVideoController.cpp:197:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/win/FullscreenVideoController.cpp:198:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/win/FullscreenVideoController.cpp:200:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/win/FullscreenVideoController.cpp:201:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/win/FullscreenVideoController.cpp:202:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/win/FullscreenVideoController.cpp:204:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 8 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 55552 [details]
Redefinition of PlatformMedia

>  #else
>  class QTMovie;
>  #endif
> +class QTMovieGWorld;
> +class QTMovieVisualContext;

Shouldn't these forward declaration be inside a platform ifdef?

>  typedef struct PlatformMedia {
> -    QTMovie* qtMovie;
> +    enum {
> +        None = 0,
> +        QTMovieType,
> +        QTMovieGWorldType,
> +        QTMovieVisualContextType
> +    } type;
> +
> +    union {
> +        QTMovie* qtMovie;
> +        QTMovieGWorld* qtMovieGWorld;
> +        QTMovieVisualContext* qtMovieVisualContext;
> +    } media;
>  } PlatformMedia;
>  
> -static const PlatformMedia NoPlatformMedia = { 0 };
> +static const PlatformMedia NoPlatformMedia = { PlatformMedia::None, {0} };

I don't think you need the "{0}" here. Do you?

It's not appropriate to have a static const object in a header file. The "static" means the object will have internal linkage and a separate copy of the object will be used in file that includes the header. Another idiom would be better.

We shouldn't use "typedef struct { } StructName" in our C++ files. Instead "struct StructName" is the style normally used.

I don't think there's any reason to explicitly make "None" be 0. It will automatically be 0 since it's the first enum value.

Since this is C++ we could use a class with constructors to make this a bit more foolproof, although this is probably fine.

> -    PlatformMedia plaftformMedia = { m_qtMovie.get() };
> +    PlatformMedia plaftformMedia;
> +		platformMedia.type = PlatformMedia::QTMovieType;
> +		platformMedia.media.qtMovie = m_qtMovie;

The tabs in this file means it can't be checked in as-is.

> -    return m_mediaElement ? reinterpret_cast<QTMovieGWorld*>(m_mediaElement->platformMedia().qtMovie) : 0;
> +		if (!m_mediaElement)
> +			return 0;
> +
> +		PlatformMedia platformMedia = m_mediaElement->platformMedia();
> +		if (platformMedia.type != PlatformMedia::QTMovieGWorldType)
> +			return 0;
> +
> +		return platformMedia.media.qtMovieGWorld;

Tabs again.

review- because of the tabs

[Jer Noble]

(In reply to comment #6)
> (From update of attachment 55552 [details])

> Shouldn't these forward declaration be inside a platform ifdef?
>

No, because in order to compile on all platforms, the typedefs need to be at least forward declared in all platforms. 

> > -static const PlatformMedia NoPlatformMedia = { 0 };
> > +static const PlatformMedia NoPlatformMedia = { PlatformMedia::None, {0} };
> 
> I don't think you need the "{0}" here. Do you?

That notation will fill the entire union with zeros. I thunk that, at least under visual studio, both the type and media fields need to be included. 

> It's not appropriate to have a static const object in a header file. The "static" means the object will have internal linkage and a separate copy of the object will be used in file that includes the header. Another idiom would be better.

How about "external const NoPlatfprmMedia;"?

> We shouldn't use "typedef struct { } StructName" in our C++ files. Instead "struct StructName" is the style normally used.

Sure thing.


> I don't think there's any reason to explicitly make "None" be 0. It will automatically be 0 since it's the first enum value.

It's my reflex to explicitly define the first entry in an enum, if only for readability's sake. I'm happy to omit it. 

> Since this is C++ we could use a class with constructors to make this a bit more foolproof, although this is probably fine.

I thought about adding constructors and accessors for watch of the subtypes, but thought it would be too verbose. 

> The tabs in this file means it can't be checked in as-is.

I'll upload a new patch with changes and without tabs. Thanks!

[Jer Noble]

(In reply to comment #7)
> That notation will fill the entire union with zeros. I thunk that, at least under visual studio, both the type and media fields need to be included. 

Apparently, I "thunk" wrong.  A test program verified that empty fields in a struct initializer are set to zero, both with cl.exe and with g++.  I'll leave that out in my patch.

[Jer Noble]

Created attachment 55565 [details]
Redefinition of PlatformMedia

[Sam Weinig]

Comment on attachment 55565 [details]
Redefinition of PlatformMedia


>  void FullscreenVideoController::setMediaElement(HTMLMediaElement* mediaElement)
WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:606
 +          platformMedia.media.qtMovie = m_qtMovie;
Some thing odd here.

WebKit/win/FullscreenVideoController.cpp:204
 +          return platformMedia.media.qtMovieGWorld;
Spacing again.

r=me with those changes.

[Jer Noble]

Created attachment 55657 [details]
Redefinition of PlatformMedia

I discovered a few compilation errors in the mac side of the previous patch, which this patch supersedes.

[Jer Noble]

Created attachment 55658 [details]
Redefinition of PlatformMedia

...and somehow the changes Darin asked for managed to escape as well.

[WebKit Review Bot]

Attachment 55658 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/win/FullscreenVideoController.cpp:197:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 7 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Attachment 55658 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2166142

[Jer Noble]

cc1plus: warnings being treated as errors
/Users/eseidel/Projects/MacEWS/WebCore/platform/graphics/MediaPlayer.cpp:66: warning: missing initializer for member 'WebCore::PlatformMedia::media'

Looks like my experiment with Darin's change was incorrect; both fields must be specified during initialization.  I'll fix and add a new patch.

[Jer Noble]

Created attachment 55662 [details]
Redefinition of PlatformMedia

Removed the random ^M newlines, as well as added an explicit initializer for NoPlatformMedia.

[Jer Noble]

Comment on attachment 55662 [details]
Redefinition of PlatformMedia

Clearing flags on attachment: 55662

Committed r59232: <http://trac.webkit.org/changeset/59232>

[Jer Noble]

All reviewed patches have been landed.  Closing bug.