Bug 43673

Summary: The preload attribute of the video tag is not completely implemented
Product: WebKit Reporter: Thomas Kupka <kupka@L3S.de>
Component: Media ElementsAssignee: Eric Carlson <eric.carlson@apple.com>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar@webkit.org, ap@webkit.org, dbates@webkit.org, eric.carlson@apple.com, mathias@qiwi.be, webkit.review.bot@gmail.com
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh Intel   
OS: Mac OS X 10.6   
Bug Depends on: 59958    
Bug Blocks:    
Attachments:
Description Flags
Obsolete patch
none
Manual test for 'preload' attribute.
beidson: review+
Patch for QTKit based media engine.
beidson: review+
Patch for AVFoundation based media engine
none
Updated patch aroben: review+

Description From 2010-08-07 08:54:36 PST
The preload attribute of the video tag works according to the standard for the values "none" and "auto". Unfortunately the value "metadata" doesn't work as expected because the whole video is loaded instead of loading only enough data to show a frame and to determine duration.

See
http://www.w3.org/TR/html5/video.html#attr-media-preload
and
http://dev.opera.com/articles/view/everything-you-need-to-know-about-html5-video-and-audio/
------- Comment #1 From 2010-08-08 15:09:54 PST -------
This is currently blocked on QuickTime, which doesn't have API to only load a portion of a movie.
------- Comment #2 From 2010-08-08 15:10:18 PST -------
rdar://7508322
------- Comment #3 From 2011-04-28 20:07:44 PST -------
Created an attachment (id=91624) [details]
Proposed patch
------- Comment #4 From 2011-04-28 20:10:53 PST -------
Attachment 91624 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:246:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:616:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #5 From 2011-04-29 07:54:56 PST -------
Splitting patch into three parts: the manual test, the changes when using AVFoundation, and the changes when using QTKit.
------- Comment #6 From 2011-04-29 07:56:03 PST -------
Created an attachment (id=91681) [details]
Manual test for 'preload' attribute.
------- Comment #7 From 2011-04-29 07:57:59 PST -------
Created an attachment (id=91682) [details]
Patch for QTKit based media engine.
------- Comment #8 From 2011-04-29 07:58:47 PST -------
Created an attachment (id=91683) [details]
Patch for AVFoundation based media engine
------- Comment #9 From 2011-04-29 13:53:20 PST -------
(From update of attachment 91683 [details])
The code here seems fine, and what I understand seems fine, but I think a domain expert should take a look - let's get an unofficial review from Jer on-up-in-here!
------- Comment #10 From 2011-04-29 14:06:01 PST -------
(From update of attachment 91683 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=91683&action=review

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.h:-113
> -    virtual bool videoLayerIsReadyToDisplay() const;

Looks like there's still a reference to videoLayerIsReadyToDisplay in MediaPlayerPrivateAVFoundation::hasAvailableVideoFrame().

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:342
> -    if (buffered()->contain(duration()))
> +    if ([m_avPlayerItem.get() isPlaybackBufferFull])

I was under the impression that looking through the buffered ranges is recommended (as opposed to calling isPlaybackBufferEmpty.  But if you're changing this for performance reasons, you should probably also change:

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:344
>          return MediaPlayerPrivateAVFoundation::MediaPlayerAVPlayerItemStatusPlaybackBufferFull;
>      if (buffered()->contain(currentTime()))

...this to [m_avPlayerItem.get() isPlaybackBufferEmpty].
------- Comment #11 From 2011-04-29 14:07:29 PST -------
(From update of attachment 91683 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=91683&action=review

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.h:-113

> 
> Looks like there's still a reference to videoLayerIsReadyToDisplay in MediaPlayerPrivateAVFoundation::hasAvailableVideoFrame().

Ah, nevermind, I see that this in in the ObjC subclass.
------- Comment #12 From 2011-04-29 14:17:48 PST -------
(From update of attachment 91683 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=91683&action=review

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:282
> -    if (m_cachedDuration == invalidTime) {
> -        m_cachedDuration = platformDuration();
> +    float duration = platformDuration();
> +    if (!duration) {
> +        m_cachedDuration = duration;

Could we perhaps return "invalidTime" instead of 0 from platformDuration()?  I realize it's unlikely for 0 to be a valid duration for a piece of media, but invalidTime would be even less valid. :)

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:240
> -    return (m_videoLayer && [m_videoLayer.get() isReadyForDisplay]);
> +    return (m_videoFrameHasDrawn || m_videoLayer && [m_videoLayer.get() isReadyForDisplay]);

I'm not entirely certain about the order of precedence here.  Boolean operators are probably LTR, but perhaps some parens for explicitness sake?  (Relevant: googling "order of precedence" leads not to C or C++ documentation, but rather rules of succession for Wales and England.)

That's all I've got.  The rest looks good!
------- Comment #13 From 2011-04-29 14:25:09 PST -------
(In reply to comment #10)
> (From update of attachment 91683 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91683&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:342
> > -    if (buffered()->contain(duration()))
> > +    if ([m_avPlayerItem.get() isPlaybackBufferFull])
> 
> I was under the impression that looking through the buffered ranges is recommended (as opposed to calling isPlaybackBufferEmpty.  But if you're changing this for performance reasons, you should probably also change:
> 

"buffered()->contain(duration())" and "[m_avPlayerItem.get() isPlaybackBufferFull]" don't tell us the same thing - the former only tells us that we have a buffer with the sample at time==duration, while the later says that AVFoundation's internal buffers are full and it has stopped buffering. We need to know when it stops buffering so we can post the 'suspend' event.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:344
> >          return MediaPlayerPrivateAVFoundation::MediaPlayerAVPlayerItemStatusPlaybackBufferFull;
> >      if (buffered()->contain(currentTime()))
> 
> ...this to [m_avPlayerItem.get() isPlaybackBufferEmpty].

Good point!
------- Comment #14 From 2011-04-29 14:35:18 PST -------
(From update of attachment 91683 [details])
Seems fine addressing Jer's feedback as needed.  *please* add those parenthesis for clarity in the 3-parter if clause.
------- Comment #15 From 2011-05-02 09:02:35 PST -------
Manual test: http://trac.webkit.org/changeset/85476
------- Comment #16 From 2011-05-02 09:09:03 PST -------
QTKit engine changes: http://trac.webkit.org/changeset/85478
------- Comment #17 From 2011-05-02 10:22:37 PST -------
AVFoundation engine changes: http://trac.webkit.org/changeset/85483
------- Comment #18 From 2011-05-03 14:14:56 PST -------
(From update of attachment 91683 [details])
http://trac.webkit.org/changeset/85483 was rolled out in bug 59958 because it caused Lion layout test regressions.
------- Comment #19 From 2011-05-03 14:16:43 PST -------
Created an attachment (id=92129) [details]
Updated patch
------- Comment #20 From 2011-05-03 17:09:44 PST -------
Re-opening this bug since one patch associated with this bug was rolled out as per comment 18.
------- Comment #21 From 2011-05-05 08:22:40 PST -------
(From update of attachment 92129 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=92129&action=review

It might have been possible to break this patch up into slightly smaller pieces, though I'm sure you tried to break it up already.

Why does only MediaPlayerPrivateAVFoundation need all this work, and not other MediaPlayerPrivate implementations?

> Source/WebCore/ChangeLog:9
> +2011-05-02  Eric Carlson  <eric.carlson@apple.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        The preload attribute of the video tag is not completely implemented
> +        https://bugs.webkit.org/show_bug.cgi?id=43673
> +        <rdar://problem/7508322>
> +
> +        Tested manually with manual-tests/media-elements/video-preload.html.

It would be nice if this explained a little more what parts of "preload" aren't implemented, and what parts this patch implements.

> Source/WebCore/ChangeLog:15
> +        (WebCore::HTMLMediaElement::seek): Call prepareToPlay when preload is less than 'metadata'

Do you mean "less than 'auto'"?

> Source/WebCore/manual-tests/media-elements/video-preload.html:100
> -        function setURL(url, vidID)
> +        function setURL(url)
>          {
> -            var vid = document.getElementById(vidID);
> +            var vid = document.getElementById("vid");

None of the changes in this file are mentioned in your ChangeLog.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:245
> +    LOG(Media, "MediaPlayerPrivateAVFMac::duration(%p) - caching %f", this, m_cachedDuration);

Not sure what the "Mac" is about here.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:395
> -        AVAssetStatus avAssetStatus = assetStatus();
> +        AssetStatus avAssetStatus = assetStatus();

Maybe this should just be called assetStatus now? (You can use this->assetStatus() on the right-hand side.)

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:437
>                  case MediaPlayerAVPlayerItemStatusPlaybackBufferFull:
> +                    m_networkState = MediaPlayer::Idle;
> +
> +                case MediaPlayerAVPlayerItemStatusReadyToPlay:

Did you intentionally leave out a "break" here?

Maybe it would be clearer to put the m_networkState logic after the switch. That way the switch can be all about m_readyState, and the code just after the switch (which is already dealing with m_networkState) can be solely in charge of m_networkState.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:461
> +        if (m_readyState < MediaPlayer::HaveCurrentData)
> +            m_readyState = MediaPlayer::HaveCurrentData;

You could use max() here, but I'm not sure if it would be clearer.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:619
> +            // AVFoundation can't open arbitrary data pointers, so if this ApplicationCacheResource doesn't 
> +            // have a valid local path, just open the resource's original URL.
> +            if (resource->path().isEmpty())
> +                createAVAssetForURL(resource->url());
> +            else
> +                createAVAssetForCacheResource(resource);

That sounds like a deficiency of AVFoundation. Maybe we should file a bug and reference it here with a FIXME?

I think it was clearer when this logic was inside createAVAssetForCacheResource. Why move it here?

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:269
> +    ASSERT(!resource->path().isEmpty());

This could be ASSERT_ARG.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:295
> +    const double veryLongInterval = 60*60*60*24*30;

1800 days? I think this would be a little clearer if written like this: 60 * 60 * 24 * 1800

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:315
> +    // Create the player item so we can media data. 

Typo: so we can media data

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:318
> +    [[NSNotificationCenter defaultCenter] addObserver:m_objcObserver.get()selector:@selector(didEnd:) name:AVPlayerItemDidPlayToEndTimeNotification object:m_avPlayerItem.get()];

Missing a space before "selector:".

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:419
> +    // Check the avitem if we have one, some assets never report duration.

Maybe "AVItem"?

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:432
> +    LOG(Media, "MediaPlayerPrivateAVFoundationObjC::duration(%p) - invalid duration, returning -1", this);
> +    return invalidTime();

Maybe you should use invalidTime() in the LOG, too, so they can't get out of sync if we change the definition of invalidTime().

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:722
> +        for (AVPlayerItemTrack *track in tracks) {
> +            if ([track isEnabled]) {
> +                AVAssetTrack *assetTrack = [track assetTrack];
> +                if ([[assetTrack mediaType] isEqualToString:AVMediaTypeVideo])
> +                    hasVideo = true;
> +                else if ([[assetTrack mediaType] isEqualToString:AVMediaTypeAudio])
> +                    hasAudio = true;
> +                else if ([[assetTrack mediaType] isEqualToString:AVMediaTypeClosedCaption])
> +                    hasCaptions = true;
> +            }
>          }

We could break out of this loop early once hasVideo && hasAudio && hasCaptions is true.
------- Comment #22 From 2011-05-05 09:15:04 PST -------
(In reply to comment #21)
> (From update of attachment 92129 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92129&action=review
> 
> It might have been possible to break this patch up into slightly smaller pieces, though I'm sure you tried to break it up already.
> 
> Why does only MediaPlayerPrivateAVFoundation need all this work, and not other MediaPlayerPrivate implementations?
> 
  QTKit is a *much* higher level API, which makes it much simpler to use in some ways but *much* harder to control in other ways. AVFoundation allows us much more control over how playback works, but a consequence is that we have to do more work ;-)


> > Source/WebCore/ChangeLog:9
> > +2011-05-02  Eric Carlson  <eric.carlson@apple.com>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        The preload attribute of the video tag is not completely implemented
> > +        https://bugs.webkit.org/show_bug.cgi?id=43673
> > +        <rdar://problem/7508322>
> > +
> > +        Tested manually with manual-tests/media-elements/video-preload.html.
> 
> It would be nice if this explained a little more what parts of "preload" aren't implemented, and what parts this patch implements.
> 
  Good point.

> > Source/WebCore/ChangeLog:15
> > +        (WebCore::HTMLMediaElement::seek): Call prepareToPlay when preload is less than 'metadata'
> 
> Do you mean "less than 'auto'"?
> 
  Fixed.

> > Source/WebCore/manual-tests/media-elements/video-preload.html:100
> > -        function setURL(url, vidID)
> > +        function setURL(url)
> >          {
> > -            var vid = document.getElementById(vidID);
> > +            var vid = document.getElementById("vid");
> 
> None of the changes in this file are mentioned in your ChangeLog.
> 
  Oops, fixed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:245
> > +    LOG(Media, "MediaPlayerPrivateAVFMac::duration(%p) - caching %f", this, m_cachedDuration);
> 
> Not sure what the "Mac" is about here.
> 
  That was the original class name, somehow the log string was never updated and nobody ever noticed. Fixed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:395
> > -        AVAssetStatus avAssetStatus = assetStatus();
> > +        AssetStatus avAssetStatus = assetStatus();
> 
> Maybe this should just be called assetStatus now? (You can use this->assetStatus() on the right-hand side.)
> 
  Good idea, fixed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:437
> >                  case MediaPlayerAVPlayerItemStatusPlaybackBufferFull:
> > +                    m_networkState = MediaPlayer::Idle;
> > +
> > +                case MediaPlayerAVPlayerItemStatusReadyToPlay:
> 
> Did you intentionally leave out a "break" here?
> 
> Maybe it would be clearer to put the m_networkState logic after the switch. That way the switch can be all about m_readyState, and the code just after the switch (which is already dealing with m_networkState) can be solely in charge of m_networkState.
> 
  Good suggestion, changed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:295
> > +    const double veryLongInterval = 60*60*60*24*30;
> 
> 1800 days? I think this would be a little clearer if written like this: 60 * 60 * 24 * 1800
> 
  Fixed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:315
> > +    // Create the player item so we can media data. 
> 
> Typo: so we can media data
> 
  Fixed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:318
> > +    [[NSNotificationCenter defaultCenter] addObserver:m_objcObserver.get()selector:@selector(didEnd:) name:AVPlayerItemDidPlayToEndTimeNotification object:m_avPlayerItem.get()];
> 
> Missing a space before "selector:".
> 
  Fixed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:419
> > +    // Check the avitem if we have one, some assets never report duration.
> 
> Maybe "AVItem"?
> 
  Fixed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:432
> > +    LOG(Media, "MediaPlayerPrivateAVFoundationObjC::duration(%p) - invalid duration, returning -1", this);
> > +    return invalidTime();
> 
> Maybe you should use invalidTime() in the LOG, too, so they can't get out of sync if we change the definition of invalidTime().
> 
  Fixed.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:722
> > +        for (AVPlayerItemTrack *track in tracks) {
> > +            if ([track isEnabled]) {
> > +                AVAssetTrack *assetTrack = [track assetTrack];
> > +                if ([[assetTrack mediaType] isEqualToString:AVMediaTypeVideo])
> > +                    hasVideo = true;
> > +                else if ([[assetTrack mediaType] isEqualToString:AVMediaTypeAudio])
> > +                    hasAudio = true;
> > +                else if ([[assetTrack mediaType] isEqualToString:AVMediaTypeClosedCaption])
> > +                    hasCaptions = true;
> > +            }
> >          }
> 
> We could break out of this loop early once hasVideo && hasAudio && hasCaptions is true.

We could, but we are very unlikely to see movies with more than 2 or 3 tracks so I think it would just add overhead to the loop.

Thanks for the review!
------- Comment #23 From 2011-05-05 11:19:46 PST -------
http://trac.webkit.org/changeset/85865
------- Comment #24 From 2011-06-03 14:11:40 PST -------
Revision r85865 cherry-picked into qtwebkit-2.2 with commit df47be2 <http://gitorious.org/webkit/qtwebkit/commit/df47be2>