Bug 134894 – Disable ff/rw based on canPlayFastForward and canPlayFastRewind.

• Component: Media
• Status: RESOLVED FIXED

[Jeremy Jones]

Disable ff/rw based on canPlayFastForward and canPlayFastRewind.

[Jeremy Jones]

<rdar://problem/17441240>

[Jeremy Jones]

Created attachment 234900 [details]
Patch

[Eric Carlson]

Comment on attachment 234900 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:3109
> +#if PLATFORM(IOS)
> +    if (!canPlayFastReverse() && rate  < 0)
> +        rate = 0;
> +    if (!canPlayFastForward() && rate > 2)
> +        rate = 2;
> +#endif

This is the wrong place to hard code maximum and minimum speeds, I doubt every platform will have the same max/min. Instead of canPlayFastForward and canPlayFastReverse, why don't you add something like maximumFastForwardRate and minimumFastReverseRate?

Also, I think 0 and 2 are the wrong values even for AVFoundation. The header docs for -[AVPlayerItem canPlayFastReverse] and -[AVPlayerItem canPlayFastForward] they indicate if an item is limited to +1 and -1.

> Source/WebCore/html/HTMLMediaElement.cpp:4866
> +void HTMLMediaElement::mediaPlayerCanPlayFastReverseChanged(MediaPlayer*)
> +{
> +    scheduleEvent(eventNames().canplayfastreversechangeEvent);
> +}

We can't notify this way, this non-standard event will be visible to scripts.

> Source/WebCore/html/HTMLMediaElement.h:542
> +    virtual void mediaPlayerCanPlayFastReverseChanged(MediaPlayer*) override;

Do we only care when the reverse rate minimum changes, what about max forward rate?

I am not sure we need to a new callback (or two) just for this, couldn't you use mediaPlayerCharacteristicChanged and check for a change?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:782
> +    [m_avPlayer.get() addObserver:m_objcObserver.get() forKeyPath:@"canPlayFastReverse" options:NSKeyValueObservingOptionNew context:(void *)MediaPlayerAVFoundationObservationContextPlayer];
> +    [m_avPlayer.get() addObserver:m_objcObserver.get() forKeyPath:@"canPlayFastForward" options:NSKeyValueObservingOptionNew context:(void *)MediaPlayerAVFoundationObservationContextPlayer];

These new observers must be removed in MediaPlayerPrivateAVFoundationObjC::cancelLoad.

[Jeremy Jones]

Created attachment 235195 [details]
Patch

[Darin Adler]

Comment on attachment 235195 [details]
Patch

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

While I’m not an expert on our media code, I think I understand this well enough to say review+

> Source/WebCore/html/HTMLMediaElement.h:192
> +    double minFastReverseRate();
> +    double maxFastForwardRate();

Should be const, I think.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:898
> +    return m_private ? m_private->maxFastForwardRate() : DBL_MAX;

In C++ code we’d normally use std::numeric_limits instead of old school numeric limit MACROS. This one would be std::numeric_limits<double>::max(). Or you could use an infinity instead. Also available from numeric_limits.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:903
> +    return m_private ? m_private->minFastReverseRate() : -DBL_MAX;

This one would be -std::numeric_limits<double>::max(). Or -infinity.

> Source/WebCore/platform/graphics/MediaPlayer.h:484
> +    double minFastReverseRate();
> +    double maxFastForwardRate();

Should be const, I think.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:115
> +    virtual double maxFastForwardRate() const override { return m_cachedCanPlayFastForward ? DBL_MAX : 2.0; }
> +    virtual double minFastReverseRate() const override { return m_cachedCanPlayFastReverse ? -DBL_MAX : 0.0; }

Same comments about DBL_MAX. Also, new overrides can and should be private unless they need to be called non-polymorphically.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2698
> +        } else if ([keyPath isEqualToString:@"canPlayFastReverse"])
> +            function = WTF::bind(&MediaPlayerPrivateAVFoundationObjC::canPlayFastReverseDidChange, m_callback, [newValue boolValue]);
> +        else if ([keyPath isEqualToString:@"canPlayFastForward"])
> +            function = WTF::bind(&MediaPlayerPrivateAVFoundationObjC::canPlayFastForwardDidChange, m_callback, [newValue boolValue]);

In the future we should consider using lambdas all throughout the function instead of WTF::bind.

[Darin Adler]

Comment on attachment 235195 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:111
> +        m_videoFullscreenInterface->setCanPlayFastReverse(m_mediaElement->minFastReverseRate() < 0.0);

Hmm, missed this the first time through. This seems a little strange. Why would a durationchangeEvent be the right time to evaluate or reevaluate the minimum fast reverse rate? Needs a comment I think.

[Eric Carlson]

Comment on attachment 235195 [details]
Patch

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

>> Source/WebCore/platform/graphics/MediaPlayer.cpp:898
>> +    return m_private ? m_private->maxFastForwardRate() : DBL_MAX;
> 
> In C++ code we’d normally use std::numeric_limits instead of old school numeric limit MACROS. This one would be std::numeric_limits<double>::max(). Or you could use an infinity instead. Also available from numeric_limits.

m_private is never NULL (see where MediaPlayer::loadWithNextMediaEngine calls createNullMediaPlayer).

>> Source/WebCore/platform/graphics/MediaPlayer.cpp:903
>> +    return m_private ? m_private->minFastReverseRate() : -DBL_MAX;
> 
> This one would be -std::numeric_limits<double>::max(). Or -infinity.

Ditto.

> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:120
> +    virtual double maxFastForwardRate() const { return DBL_MAX; }
> +    virtual double minFastReverseRate() const { return -DBL_MAX; }

Apply Darin's comments about using std::numeric_limits here instead.

[Jeremy Jones]

(In reply to comment #3)
> (From update of attachment 234900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234900&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3109
> > +#if PLATFORM(IOS)
> > +    if (!canPlayFastReverse() && rate  < 0)
> > +        rate = 0;
> > +    if (!canPlayFastForward() && rate > 2)
> > +        rate = 2;
> > +#endif
> 
> This is the wrong place to hard code maximum and minimum speeds, I doubt every platform will have the same max/min. Instead of canPlayFastForward and canPlayFastReverse, why don't you add something like maximumFastForwardRate and minimumFastReverseRate?

Pushed down to MediaPlayer.

> 
> Also, I think 0 and 2 are the wrong values even for AVFoundation. The header docs for -[AVPlayerItem canPlayFastReverse] and -[AVPlayerItem canPlayFastForward] they indicate if an item is limited to +1 and -1.

Per AVF docs, when canPlayFastForward is false, it can still play at 2x.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4866
> > +void HTMLMediaElement::mediaPlayerCanPlayFastReverseChanged(MediaPlayer*)
> > +{
> > +    scheduleEvent(eventNames().canplayfastreversechangeEvent);
> > +}
> 
> We can't notify this way, this non-standard event will be visible to scripts.

Removed.

> 
> > Source/WebCore/html/HTMLMediaElement.h:542
> > +    virtual void mediaPlayerCanPlayFastReverseChanged(MediaPlayer*) override;
> 
> Do we only care when the reverse rate minimum changes, what about max forward rate?

Only the rewind button gets disabled.

> 
> I am not sure we need to a new callback (or two) just for this, couldn't you use mediaPlayerCharacteristicChanged and check for a change?

I'll use duration change, since it happens at the right time.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:782
> > +    [m_avPlayer.get() addObserver:m_objcObserver.get() forKeyPath:@"canPlayFastReverse" options:NSKeyValueObservingOptionNew context:(void *)MediaPlayerAVFoundationObservationContextPlayer];
> > +    [m_avPlayer.get() addObserver:m_objcObserver.get() forKeyPath:@"canPlayFastForward" options:NSKeyValueObservingOptionNew context:(void *)MediaPlayerAVFoundationObservationContextPlayer];
> 
> These new observers must be removed in MediaPlayerPrivateAVFoundationObjC::cancelLoad.

I've removed this and instead correctly observe on AVPlayerItem.

[Jeremy Jones]

(In reply to comment #7)
> (From update of attachment 235195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235195&action=review
> 
> >> Source/WebCore/platform/graphics/MediaPlayer.cpp:898
> >> +    return m_private ? m_private->maxFastForwardRate() : DBL_MAX;
> > 
> > In C++ code we’d normally use std::numeric_limits instead of old school numeric limit MACROS. This one would be std::numeric_limits<double>::max(). Or you could use an infinity instead. Also available from numeric_limits.
> 
> m_private is never NULL (see where MediaPlayer::loadWithNextMediaEngine calls createNullMediaPlayer).
> 

removed condition and the need for DBL_MAX.

> >> Source/WebCore/platform/graphics/MediaPlayer.cpp:903
> >> +    return m_private ? m_private->minFastReverseRate() : -DBL_MAX;
> > 
> > This one would be -std::numeric_limits<double>::max(). Or -infinity.
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/platform/graphics/MediaPlayerPrivate.h:120
> > +    virtual double maxFastForwardRate() const { return DBL_MAX; }
> > +    virtual double minFastReverseRate() const { return -DBL_MAX; }
> 
> Apply Darin's comments about using std::numeric_limits here instead.

Got it.

[Jeremy Jones]

(In reply to comment #6)
> (From update of attachment 235195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235195&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:111
> > +        m_videoFullscreenInterface->setCanPlayFastReverse(m_mediaElement->minFastReverseRate() < 0.0);
> 
> Hmm, missed this the first time through. This seems a little strange. Why would a durationchangeEvent be the right time to evaluate or reevaluate the minimum fast reverse rate? Needs a comment I think.

This is a little strange:
        // These is no standard event for minFastReverseRateChange; duration change is a reasonable proxy for it.
        // It happens every time a new item becomes ready to play.

[Jeremy Jones]

(In reply to comment #5)
> (From update of attachment 235195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235195&action=review
> 
> While I’m not an expert on our media code, I think I understand this well enough to say review+
> 
> > Source/WebCore/html/HTMLMediaElement.h:192
> > +    double minFastReverseRate();
> > +    double maxFastForwardRate();
> 
> Should be const, I think.

Yup, also in MediaPlayer.h/cpp. It is already const in MediaPlayerPrivate.

> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:898
> > +    return m_private ? m_private->maxFastForwardRate() : DBL_MAX;
> 
> In C++ code we’d normally use std::numeric_limits instead of old school numeric limit MACROS. This one would be std::numeric_limits<double>::max(). Or you could use an infinity instead. Also available from numeric_limits.

Ok, infinity it is. But, this goes away per Eric's comment that m_private is never null.
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:903
> > +    return m_private ? m_private->minFastReverseRate() : -DBL_MAX;
> 
> This one would be -std::numeric_limits<double>::max(). Or -infinity.

Ditto.

> 
> > Source/WebCore/platform/graphics/MediaPlayer.h:484
> > +    double minFastReverseRate();
> > +    double maxFastForwardRate();
> 
> Should be const, I think.

Done.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:115
> > +    virtual double maxFastForwardRate() const override { return m_cachedCanPlayFastForward ? DBL_MAX : 2.0; }
> > +    virtual double minFastReverseRate() const override { return m_cachedCanPlayFastReverse ? -DBL_MAX : 0.0; }
> 
> Same comments about DBL_MAX. Also, new overrides can and should be private unless they need to be called non-polymorphically.

Done, and moved to private: section.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2698
> > +        } else if ([keyPath isEqualToString:@"canPlayFastReverse"])
> > +            function = WTF::bind(&MediaPlayerPrivateAVFoundationObjC::canPlayFastReverseDidChange, m_callback, [newValue boolValue]);
> > +        else if ([keyPath isEqualToString:@"canPlayFastForward"])
> > +            function = WTF::bind(&MediaPlayerPrivateAVFoundationObjC::canPlayFastForwardDidChange, m_callback, [newValue boolValue]);
> 
> In the future we should consider using lambdas all throughout the function instead of WTF::bind.

This goes away as this property should actually be observed on the AVPlayerItem instead of the AVPlayer.

[Jeremy Jones]

Created attachment 235196 [details]
Patch for landing.

[Jeremy Jones]

(In reply to comment #11)
> (In reply to comment #5)
> > (From update of attachment 235195 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=235195&action=review
> > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2698
> > > +        } else if ([keyPath isEqualToString:@"canPlayFastReverse"])
> > > +            function = WTF::bind(&MediaPlayerPrivateAVFoundationObjC::canPlayFastReverseDidChange, m_callback, [newValue boolValue]);
> > > +        else if ([keyPath isEqualToString:@"canPlayFastForward"])
> > > +            function = WTF::bind(&MediaPlayerPrivateAVFoundationObjC::canPlayFastForwardDidChange, m_callback, [newValue boolValue]);
> > 
> > In the future we should consider using lambdas all throughout the function instead of WTF::bind.
> 
> This goes away as this property should actually be observed on the AVPlayerItem instead of the AVPlayer.

Disregard my comment here. This does not go away; it is just moved.

[Jeremy Jones]

Created attachment 235197 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 235197 [details]
Patch for landing.

Clearing flags on attachment: 235197

Committed r171288: <http://trac.webkit.org/changeset/171288>