WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
43673
The preload attribute of the video tag is not completely implemented
https://bugs.webkit.org/show_bug.cgi?id=43673
Summary
The preload attribute of the video tag is not completely implemented
Thomas Kupka
Reported
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/
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Eric Carlson
Comment 1
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.
Eric Carlson
Comment 2
2010-08-08 15:10:18 PDT
rdar://7508322
Eric Carlson
Comment 3
2011-04-28 20:07:44 PDT
Created
attachment 91624
[details]
Obsolete patch
WebKit Review Bot
Comment 4
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.
Eric Carlson
Comment 5
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.
Eric Carlson
Comment 6
2011-04-29 07:56:03 PDT
Created
attachment 91681
[details]
Manual test for 'preload' attribute.
Eric Carlson
Comment 7
2011-04-29 07:57:59 PDT
Created
attachment 91682
[details]
Patch for QTKit based media engine.
Eric Carlson
Comment 8
2011-04-29 07:58:47 PDT
Created
attachment 91683
[details]
Patch for AVFoundation based media engine
Brady Eidson
Comment 9
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!
Jer Noble
Comment 10
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].
Jer Noble
Comment 11
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.
Jer Noble
Comment 12
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!
Eric Carlson
Comment 13
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!
Brady Eidson
Comment 14
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.
Eric Carlson
Comment 15
2011-05-02 09:02:35 PDT
Manual test:
http://trac.webkit.org/changeset/85476
Eric Carlson
Comment 16
2011-05-02 09:09:03 PDT
QTKit engine changes:
http://trac.webkit.org/changeset/85478
Eric Carlson
Comment 17
2011-05-02 10:22:37 PDT
AVFoundation engine changes:
http://trac.webkit.org/changeset/85483
Eric Carlson
Comment 18
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.
Eric Carlson
Comment 19
2011-05-03 14:16:43 PDT
Created
attachment 92129
[details]
Updated patch
Daniel Bates
Comment 20
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
.
Adam Roben (:aroben)
Comment 21
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.
Eric Carlson
Comment 22
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!
Eric Carlson
Comment 23
2011-05-05 11:19:46 PDT
http://trac.webkit.org/changeset/85865
Ademar Reis
Comment 24
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
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug