Bug 43673 - The preload attribute of the video tag is not completely implemented
Summary: The preload attribute of the video tag is not completely implemented
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on: 59958
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-07 08:54 PDT by Thomas Kupka
Modified: 2011-06-03 14:11 PDT (History)
6 users (show)

See Also:


Attachments
Obsolete patch (48.46 KB, text/plain)
2011-04-28 20:07 PDT, Eric Carlson
no flags Details
Manual test for 'preload' attribute. (9.91 KB, patch)
2011-04-29 07:56 PDT, Eric Carlson
beidson: review+
Details | Formatted Diff | Diff
Patch for QTKit based media engine. (4.71 KB, patch)
2011-04-29 07:57 PDT, Eric Carlson
beidson: review+
Details | Formatted Diff | Diff
Patch for AVFoundation based media engine (27.34 KB, patch)
2011-04-29 07:58 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch (42.59 KB, patch)
2011-05-03 14:16 PDT, Eric Carlson
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Kupka 2010-08-07 08:54:36 PDT
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 Eric Carlson 2010-08-08 15:09:54 PDT
This is currently blocked on QuickTime, which doesn't have API to only load a portion of a movie.
Comment 2 Eric Carlson 2010-08-08 15:10:18 PDT
rdar://7508322
Comment 3 Eric Carlson 2011-04-28 20:07:44 PDT
Created attachment 91624 [details]
Obsolete patch
Comment 4 WebKit Review Bot 2011-04-28 20:10:53 PDT
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 Eric Carlson 2011-04-29 07:54:56 PDT
Splitting patch into three parts: the manual test, the changes when using AVFoundation, and the changes when using QTKit.
Comment 6 Eric Carlson 2011-04-29 07:56:03 PDT
Created attachment 91681 [details]
Manual test for 'preload' attribute.
Comment 7 Eric Carlson 2011-04-29 07:57:59 PDT
Created attachment 91682 [details]
Patch for QTKit based media engine.
Comment 8 Eric Carlson 2011-04-29 07:58:47 PDT
Created attachment 91683 [details]
Patch for AVFoundation based media engine
Comment 9 Brady Eidson 2011-04-29 13:53:20 PDT
Comment on attachment 91683 [details]
Patch for AVFoundation based media engine

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 Jer Noble 2011-04-29 14:06:01 PDT
Comment on attachment 91683 [details]
Patch for AVFoundation based media engine

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 Jer Noble 2011-04-29 14:07:29 PDT
Comment on attachment 91683 [details]
Patch for AVFoundation based media engine

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 Jer Noble 2011-04-29 14:17:48 PDT
Comment on attachment 91683 [details]
Patch for AVFoundation based media engine

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 Eric Carlson 2011-04-29 14:25:09 PDT
(In reply to comment #10)
> (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.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 Brady Eidson 2011-04-29 14:35:18 PDT
Comment on attachment 91683 [details]
Patch for AVFoundation based media engine

Seems fine addressing Jer's feedback as needed.  *please* add those parenthesis for clarity in the 3-parter if clause.
Comment 15 Eric Carlson 2011-05-02 09:02:35 PDT
Manual test: http://trac.webkit.org/changeset/85476
Comment 16 Eric Carlson 2011-05-02 09:09:03 PDT
QTKit engine changes: http://trac.webkit.org/changeset/85478
Comment 17 Eric Carlson 2011-05-02 10:22:37 PDT
AVFoundation engine changes: http://trac.webkit.org/changeset/85483
Comment 18 Eric Carlson 2011-05-03 14:14:56 PDT
Comment on attachment 91683 [details]
Patch for AVFoundation based media engine

http://trac.webkit.org/changeset/85483 was rolled out in bug 59958 because it caused Lion layout test regressions.
Comment 19 Eric Carlson 2011-05-03 14:16:43 PDT
Created attachment 92129 [details]
Updated patch
Comment 20 Daniel Bates 2011-05-03 17:09:44 PDT
Re-opening this bug since one patch associated with this bug was rolled out as per comment 18.
Comment 21 Adam Roben (:aroben) 2011-05-05 08:22:40 PDT
Comment on attachment 92129 [details]
Updated patch

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 Eric Carlson 2011-05-05 09:15:04 PDT
(In reply to comment #21)
> (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?
> 
  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 Eric Carlson 2011-05-05 11:19:46 PDT
http://trac.webkit.org/changeset/85865
Comment 24 Ademar Reis 2011-06-03 14:11:40 PDT
Revision r85865 cherry-picked into qtwebkit-2.2 with commit df47be2 <http://gitorious.org/webkit/qtwebkit/commit/df47be2>