Summary: | Disable ff/rw based on canPlayFastForward and canPlayFastRewind. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeremy Jones <jeremyj-wk> | ||||||||||
Component: | Media | Assignee: | Jeremy Jones <jeremyj-wk> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, kangil.han, philipj, sergio | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | iPhone / iPad | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Jeremy Jones
2014-07-14 12:56:24 PDT
Created attachment 234900 [details]
Patch
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. Created attachment 235195 [details]
Patch
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. 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. 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. (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. (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. (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. (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. Created attachment 235196 [details]
Patch for landing.
(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. Created attachment 235197 [details]
Patch for landing.
Comment on attachment 235197 [details] Patch for landing. Clearing flags on attachment: 235197 Committed r171288: <http://trac.webkit.org/changeset/171288> |