Bug 43673 – The preload attribute of the video tag is not completely implemented

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Thomas Kupka]

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/

[Eric Carlson]

This is currently blocked on QuickTime, which doesn't have API to only load a portion of a movie.

[Eric Carlson]

rdar://7508322

[Eric Carlson]

Created attachment 91624 [details]
Obsolete patch

[WebKit Review Bot]

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.

[Eric Carlson]

Splitting patch into three parts: the manual test, the changes when using AVFoundation, and the changes when using QTKit.

[Eric Carlson]

Created attachment 91681 [details]
Manual test for 'preload' attribute.

[Eric Carlson]

Created attachment 91682 [details]
Patch for QTKit based media engine.

[Eric Carlson]

Created attachment 91683 [details]
Patch for AVFoundation based media engine

[Brady Eidson]

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!

[Jer Noble]

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].

[Jer Noble]

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.

[Jer Noble]

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!

[Eric Carlson]

(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!

[Brady Eidson]

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.

[Eric Carlson]

Manual test: http://trac.webkit.org/changeset/85476

[Eric Carlson]

QTKit engine changes: http://trac.webkit.org/changeset/85478

[Eric Carlson]

AVFoundation engine changes: http://trac.webkit.org/changeset/85483

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 92129 [details]
Updated patch

[Daniel Bates]

Re-opening this bug since one patch associated with this bug was rolled out as per comment 18.

[Adam Roben (:aroben)]

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
> [phone]  Eric Carlson  <[e-mail]>
> +
> +        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.

[Eric Carlson]

(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
> > [phone]  Eric Carlson  <[e-mail]>
> > +
> > +        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!

[Eric Carlson]

http://trac.webkit.org/changeset/85865

[Ademar Reis]

Revision r85865 cherry-picked into qtwebkit-2.2 with commit df47be2 <http://gitorious.org/webkit/qtwebkit/commit/df47be2>