Bug 128684 – Implement hasLiveStreamingContent property in WebAVPlayerController

• Component: Media
• Status: RESOLVED FIXED

[Jeremy Jones]

Implementing hasLiveStreamingContent will enable the live streaming UI.

[Radar WebKit Bug Importer]

<rdar://problem/16358957>

[Jeremy Jones]

Created attachment 227474 [details]
Patch

[Jeremy Jones]

Simon,
One question, in the IPC interface I convert a TimeRange to a Vector instead of creating an encoder/decoder for TimeRange. Would it be preferable to create the encoder/decoder?

[Simon Fraser (smfr)]

Comment on attachment 227474 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:56
> +    virtual void setSeekableRangesVector(Vector<std::pair<double, double>>);

Pass a reference unless you're using std::move.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:70
> +    

Blank space.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:71
> +    for (auto range : ranges)

const auto&

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:72
> +        timeRanges->add(range.first, range.second);

You should add some basic validity checking here (check that range end is > range start, and any other invariants). Basically you should assume that data coming over IPC is unsafe.

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:73
> +    virtual void setSeekableRanges(WebCore::TimeRanges&) override;

const WebCore::TimeRanges&

[Jeremy Jones]

(In reply to comment #4)
> (From update of attachment 227474 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227474&action=review
> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:56
> > +    virtual void setSeekableRangesVector(Vector<std::pair<double, double>>);
> 
> Pass a reference unless you're using std::move.

Passing Vector<> by reference is unprecedented in IPC interface, so I'll change the one call site to use std::move.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:70
> > +    
> 
> Blank space.

Removed.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:71
> > +    for (auto range : ranges)
> 
> const auto&

Done.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:72
> > +        timeRanges->add(range.first, range.second);
> 
> You should add some basic validity checking here (check that range end is > range start, and any other invariants). Basically you should assume that data coming over IPC is unsafe.

Done

> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:73
> > +    virtual void setSeekableRanges
(WebCore::TimeRanges&) override;
> 
> const WebCore::TimeRanges&

Done.

[Jeremy Jones]

Created attachment 227512 [details]
Patch

[Eric Carlson]

Comment on attachment 227512 [details]
Patch

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

This looks OK to me modulo my minor comments, but I am now a WK2 reviewer.

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:113
> +        m_videoFullscreenInterface->setSeekableRanges(*m_mediaElement->seekable());

This is OK for now, but please add a FIXME and file a bug about coming up with a more efficient method of updating ranges than polling 3 times per second.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:79
> +    setSeekableRanges(*timeRanges);

Could you pass the Vector<> directly instead of creating a temporary TimeRanges?

[Eric Carlson]

(In reply to comment #7)
> (From update of attachment 227512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227512&action=review
> 
> This looks OK to me modulo my minor comments, but I am now a WK2 reviewer.
> 
By which I meant I am *not* a WK2 reviewer, of course.

[Simon Fraser (smfr)]

Comment on attachment 227512 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterface.h:33
> +#import <WebCore/TimeRanges.h>

Don't include the header unless you need the implementation. Instead, forward-declare the class. Same with PlatformLayer.h if that's fixable.

We have huge build time problems because of over-eager #includes.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:365
> +void WebVideoFullscreenInterfaceAVKit::setSeekableRanges(const WebCore::TimeRanges& timeRanges)

Need the WebCore::?

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:112
>          m_videoFullscreenInterface->setCurrentTime(m_mediaElement->currentTime(), [[NSProcessInfo processInfo] systemUptime]);

What is the [[NSProcessInfo processInfo] systemUptime] thing for? Weird.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:56
> +    virtual void setSeekableRangesVector(Vector<std::pair<double, double>>);

Vector&?

[Jeremy Jones]

(In reply to comment #7)
> (From update of attachment 227512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227512&action=review
> 
> This looks OK to me modulo my minor comments, but I am now a WK2 reviewer.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:113
> > +        m_videoFullscreenInterface->setSeekableRanges(*m_mediaElement->seekable());
> 
> This is OK for now, but please add a FIXME and file a bug about coming up with a more efficient method of updating ranges than polling 3 times per second.

I added a change to use the progressEvent.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:79
> > +    setSeekableRanges(*timeRanges);
> 
> Could you pass the Vector<> directly instead of creating a temporary TimeRanges?

The is one of a couple solutions.
1) Create an encoder/decoder for TimeRanges to they can cross IPC boundary. Thus removing the vector.
2) Pass Vector<> directly, which will unnecessarily convert TimeRanges to a Vector<> in WK1.

I wrote https://bugs.webkit.org/show_bug.cgi?id=130784 to follow up on this.

[Jeremy Jones]

(In reply to comment #9)
> (From update of attachment 227512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227512&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterface.h:33
> > +#import <WebCore/TimeRanges.h>
> 
> Don't include the header unless you need the implementation. Instead, forward-declare the class. Same with PlatformLayer.h if that's fixable.
> 
> We have huge build time problems because of over-eager #includes.


Removed include of TimeRanges.h and added it to the appropriate .m files.
Removed include of PlatformLayer.h

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:365
> > +void WebVideoFullscreenInterfaceAVKit::setSeekableRanges(const WebCore::TimeRanges& timeRanges)
> 
> Need the WebCore::?

Removed.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:112
> >          m_videoFullscreenInterface->setCurrentTime(m_mediaElement->currentTime(), [[NSProcessInfo processInfo] systemUptime]);
> 
> What is the [[NSProcessInfo processInfo] systemUptime] thing for? Weird.

It means, currentTime was X when systemUptime was Y. You can use this and the current rate to extrapolate the current time more accurately and continuously. Then you just periodically update this to minimize drift.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:56
> > +    virtual void setSeekableRangesVector(Vector<std::pair<double, double>>);
> 
> Vector&?

Aha! The IPC interface can be declared without the reference, but this implementation can still use a reference because the call syntax is the same.

Done.

[Jeremy Jones]

(In reply to comment #10)
> (In reply to comment #7)
> > (From update of attachment 227512 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=227512&action=review
> > 
> > This looks OK to me modulo my minor comments, but I am now a WK2 reviewer.
> > 
> > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:113
> > > +        m_videoFullscreenInterface->setSeekableRanges(*m_mediaElement->seekable());
> > 
> > This is OK for now, but please add a FIXME and file a bug about coming up with a more efficient method of updating ranges than polling 3 times per second.
> 
> I added a change to use the progressEvent.

progressEvent isn't the correct event. I've added a FIXME, and filed https://bugs.webkit.org/show_bug.cgi?id=130788

[Jeremy Jones]

Created attachment 227871 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 227871 [details]
Patch

Rejecting attachment 227871 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 227871, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/6248663194009600

[Eric Carlson]

(In reply to comment #12)
> progressEvent isn't the correct event. I've added a FIXME, and filed https://bugs.webkit.org/show_bug.cgi?id=130788

Thanks.

[Jeremy Jones]

Created attachment 227875 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 227875 [details]
Patch

Clearing flags on attachment: 227875

Committed r166312: <http://trac.webkit.org/changeset/166312>