Bug 38689 - #34005 will break fullscreen video playback
Summary: #34005 will break fullscreen video playback
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://vimeo.com/7352118
Keywords:
Depends on: 34005
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-06 14:18 PDT by Jer Noble
Modified: 2010-05-12 07:35 PDT (History)
2 users (show)

See Also:


Attachments
Part 1: PlatformMedia redefinition (1003 bytes, patch)
2010-05-06 14:25 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Redefinition of PlatformMedia (7.14 KB, patch)
2010-05-10 07:28 PDT, Jer Noble
darin: review-
Details | Formatted Diff | Diff
Redefinition of PlatformMedia (7.70 KB, patch)
2010-05-10 10:18 PDT, Jer Noble
sam: review+
Details | Formatted Diff | Diff
Redefinition of PlatformMedia (7.84 KB, patch)
2010-05-10 21:24 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Redefinition of PlatformMedia (8.26 KB, patch)
2010-05-10 21:28 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Redefinition of PlatformMedia (8.23 KB, patch)
2010-05-10 21:49 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2010-05-06 14:18:12 PDT
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.
Comment 1 Jer Noble 2010-05-06 14:25:50 PDT
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.
Comment 2 Eric Seidel (no email) 2010-05-06 20:06:03 PDT
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...
Comment 3 WebKit Review Bot 2010-05-09 22:55:15 PDT
Attachment 55297 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2238076
Comment 4 Jer Noble 2010-05-10 07:28:43 PDT
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.
Comment 5 WebKit Review Bot 2010-05-10 07:35:38 PDT
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.
Comment 6 Darin Adler 2010-05-10 08:39:42 PDT
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
Comment 7 Jer Noble 2010-05-10 09:31:54 PDT
(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!
Comment 8 Jer Noble 2010-05-10 10:14:25 PDT
(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.
Comment 9 Jer Noble 2010-05-10 10:18:41 PDT
Created attachment 55565 [details]
Redefinition of PlatformMedia
Comment 10 Sam Weinig 2010-05-10 11:25:30 PDT
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.
Comment 11 Jer Noble 2010-05-10 21:24:56 PDT
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.
Comment 12 Jer Noble 2010-05-10 21:28:09 PDT
Created attachment 55658 [details]
Redefinition of PlatformMedia

...and somehow the changes Darin asked for managed to escape as well.
Comment 13 WebKit Review Bot 2010-05-10 21:34:52 PDT
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.
Comment 14 Eric Seidel (no email) 2010-05-10 21:37:02 PDT
Attachment 55658 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2166142
Comment 15 Jer Noble 2010-05-10 21:41:17 PDT
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.
Comment 16 Jer Noble 2010-05-10 21:49:25 PDT
Created attachment 55662 [details]
Redefinition of PlatformMedia

Removed the random ^M newlines, as well as added an explicit initializer for NoPlatformMedia.
Comment 17 Jer Noble 2010-05-12 07:35:26 PDT
Comment on attachment 55662 [details]
Redefinition of PlatformMedia

Clearing flags on attachment: 55662

Committed r59232: <http://trac.webkit.org/changeset/59232>
Comment 18 Jer Noble 2010-05-12 07:35:32 PDT
All reviewed patches have been landed.  Closing bug.