Bug 154742 – Add support for playbackControlsManager

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Beth Dakin]

Add support for playbackControlsManager

rdar://problem/23833753

[Beth Dakin]

Created attachment 272360 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:35:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:36:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:68:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:97:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:102:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:112:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:117:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:130:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
Total errors found: 8 in 25 files


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

[Beth Dakin]

Created attachment 272362 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:35:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:39:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:69:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:131:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
Total errors found: 4 in 25 files


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

[Sam Weinig]

Comment on attachment 272362 [details]
Patch

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

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:289
> +    uint64_t contextId;
> +
> +    auto addResult = m_videoElements.add(&videoElement, 0);
> +    if (addResult.isNewEntry)
> +        addResult.iterator->value = nextContextId();
> +    contextId = addResult.iterator->value;

There is new fangled way to do this using HashMap::ensure(..)

uint64_t contextId = m_videoElements.add(&videoElement, [&] { return nextContextId(); }).iterator->value;

or in two lines:

auto addResult = m_videoElements.add(&videoElement, [&] { return nextContextId(); });
auto contextId = addResult.iterator->value;

[Eric Carlson]

Comment on attachment 272360 [details]
Patch

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

> Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:179
> +    controlsManager.hasEnabledAudio = YES;
> +    controlsManager.hasEnabledVideo = YES;

This isn't necessarily correct, should it have a FIXME?

> Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:209
> +        double start = timeRanges.start(i, exceptionCode);
> +        double end = timeRanges.end(i, exceptionCode);

Can you use ASSERT_NO_EXCEPTION instead of ignoring potential exceptions?

> Source/WebKit2/UIProcess/mac/PageClientImpl.h:217
> +    virtual void isPlayingMediaDidChange() override;

Nit: "virtual" is implied with "override".

[Jer Noble]

Comment on attachment 272362 [details]
Patch

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

> Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:213
> +        double start = timeRanges.start(i, exceptionCode);
> +        double end = timeRanges.end(i, exceptionCode);
> +        
> +        CMTimeRange range = CMTimeRangeMake(CMTimeMakeWithSeconds(start, 1000), CMTimeMakeWithSeconds(end-start, 1000));
> +        [seekableRanges addObject:[NSValue valueWithCMTimeRange:range]];

You can use timeRanges.ranges() to get a PlatformTimeRanges. This object's start() and end()  methods don't take an exception code, and they return MediaTime objects.  So this would become:

const PlatformTimeRanges& ranges = timeRanges.ranges();
CMTimeRange range = CMTimeRangeMake(toCMTime(ranges.start(i)), toCMTime(ranges.end(i)));

> Source/WebCore/platform/spi/mac/AVFoundationSPI.h:35
> +#import <AVFoundation/AVTime.h>

AVTime.h should be available in the SDK.

[Beth Dakin]

Thanks all! I believe that I addressed all comments: http://trac.webkit.org/changeset/197204

[Beth Dakin]

Build fix: http://trac.webkit.org/changeset/197207

[Beth Dakin]

More build fix: http://trac.webkit.org/changeset/197211

[Alexey Proskuryakov]

This caused multiple crashes, will roll out. More info in e-mail.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x00000001073e4762 WebKit::WebVideoFullscreenManagerProxy::ensureModelAndInterface(unsigned long long) + 180
1   com.apple.WebKit              	0x00000001073e50a5 WebKit::WebVideoFullscreenManagerProxy::controlsManagerInterface() + 13
2   com.apple.WebKit              	0x00000001073e9e5c WebKit::WebViewImpl::updateWebViewImplAdditions() + 310
3   com.apple.WebKit              	0x00000001073730c5 WebKit::WebPageProxy::isPlayingMediaDidChange(unsigned int, unsigned long long) + 51
4   com.apple.WebKit              	0x000000010738672d WebKit::WebPageProxy::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) + 12389

[WebKit Commit Bot]

Re-opened since this is blocked by bug 154780

[Beth Dakin]

Created attachment 272680 [details]
Patch that attempts to resolve crashes

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:34:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:35:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:38:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:40:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:78:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:83:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:84:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:85:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:86:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:87:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:88:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:89:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:148:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
ERROR: Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:298:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 14 in 24 files


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

[Beth Dakin]

Thanks Jer! Giving this another go: http://trac.webkit.org/changeset/197461

[Beth Dakin]

This caused some new crashes, which I addressed with http://trac.webkit.org/changeset/197469