[Webkit-unassigned] [Bug 64731] Add MediaSource API to HTMLMediaElement
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Aug 10 15:41:55 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=64731
Eric Carlson <eric.carlson at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #102925|review?, commit-queue? |review-
Flag| |
--- Comment #25 from Eric Carlson <eric.carlson at apple.com> 2011-08-10 15:41:54 PST ---
(From update of attachment 102925)
View in context: https://bugs.webkit.org/attachment.cgi?id=102925&action=review
All of these tests are extremely WebM specific, whereas all of the existing media tests are format agnostic. This seems reasonable for now because I assume the feature only works with WebM files at the moment, but the test file names should reflect this. Or it may make more sense to make a "mediasource" directory in LayoutTests/media, and to have a folder in that for each format. This test structure will also make it easier to skip tests on ports where they aren't supported because you can just include "media/mediasource/" in the skipped file to skip all tests.
> LayoutTests/http/tests/media/media-source.js:23
> + var r = new XMLHttpRequest();
> + r.open("GET", url, false);
> + r.responseType = 'arraybuffer';
> + r.send();
Nit: We try to use descriptive names for variables in WebKit.
> LayoutTests/http/tests/media/media-source.js:25
> + if (r.status != 200) {
Nit: Although there isn't a formal standard for tab width in JS files, I believe that all of the media tests use 4-tab spaces.
> LayoutTests/http/tests/media/media-source.js:39
> +function getNumClusters() {
> + return cluster_info.length;
> +}
Nit: We try to use full names instead of abbreviations, eg. getClusterCount, getNumberOfClusters, etc.
> LayoutTests/http/tests/media/media-source.js:41
> +function getCluster(i) {
Nit: descriptive variable names are preferred. Here and throughout the rest of the file.
> LayoutTests/http/tests/media/media-source.js:119
> + case 0:
> + str = "SOURCE_CLOSED";
> + break;
> + case 1:
> + str = "SOURCE_OPEN";
> + break;
> + case 2:
Is there any reason to not use the named constants, eg. "case HTMLMediaElement.SOURCE_CLOSED:", etc?
> LayoutTests/http/tests/media/video-media-source-errors.html:1
> +<html>
Nit: Is there any reason to not include an html5 DOCTYPE? Here and elsewhere.
> LayoutTests/http/tests/media/video-media-source-errors.html:35
> + appendCluster(v, next_cluster_index);
> + window.setTimeout(f, 0, next_cluster_index + 1);
Is there any way to do this with an event listener instead of setTimeout (here and elsewhere)? Timeouts have been the source of lots of flakiness in media tests.
> LayoutTests/http/tests/media/video-media-source-seek.html:40
> + if (Math.abs(currentTime - seekTime) > 0.001) {
Why do you need the magic number? At the very least you should add a comment about why this is needed and what this tolerance is OK. How much variation do you actually see?
> LayoutTests/http/tests/media/video-media-source-state-changes.html:17
> + window.setTimeout(appendUntilEnd, 0, v, i + 1);
More setTimeout.
> LayoutTests/http/tests/media/video-media-source-state-changes.html:23
> + return
Missing a semi-colon here.
> LayoutTests/http/tests/media/video-media-source-state-changes.html:32
> + window.setTimeout(appendClustersUntilTrue, 0, v, i + 1, conditionStr, done_cb);
More setTimeout.
> LayoutTests/http/tests/media/video-media-source-state-changes.html:62
> + window.setTimeout(appendUntilEnd, 0, v, 0);
More setTimeout.
> LayoutTests/http/tests/media/video-media-source-state-changes.html:88
> + window.setTimeout(appendClustersUntilTrue, 0,
> + v, startIndex, "firstSeekComplete",
> + function () { firstSeekClusterAppendComplete(v);} );
More setTimeout.
> LayoutTests/http/tests/media/video-media-source-state-changes.html:120
> + window.setTimeout(appendClustersUntilTrue, 0,
> + v, startIndex, "secondSeekComplete",
> + function () { secondSeekClusterAppendComplete(v);} );
> + }
More setTimeout.
> LayoutTests/http/tests/media/video-media-source-state-changes.html:159
> + window.setTimeout(appendUntilEnd, 0, v, 0);
More setTimeout.
> Source/WebCore/html/HTMLMediaElement.cpp:571
> if (m_networkState != NETWORK_EMPTY) {
> +#if ENABLE(MEDIA_SOURCE)
> + setSourceState(SOURCE_CLOSED);
> +#endif
> m_networkState = NETWORK_EMPTY;
> m_readyState = HAVE_NOTHING;
Why do you set the state to SOURCE_CLOSED here...
> Source/WebCore/html/HTMLMediaElement.cpp:599
> m_networkState = NETWORK_NO_SOURCE;
>
> +#if ENABLE(MEDIA_SOURCE)
> + setSourceState(SOURCE_CLOSED);
> +#endif
> +
> // 2 - Asynchronously await a stable state.
and here?
> Source/WebCore/html/HTMLMediaElement.idl:104
> + // media source state
Comment should be a complete sentence.
> Source/WebKit/chromium/ChangeLog:19
> +2011-08-04 Aaron Colwell <acolwell at chromium.org>
> +
> + Add MediaSource API to HTMLMediaElement
> + https://bugs.webkit.org/show_bug.cgi?id=64731
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + * features.gypi:
> + * public/WebMediaPlayer.h:
> + (WebKit::WebMediaPlayer::sourceAppend):
> + (WebKit::WebMediaPlayer::sourceEndOfStream):
> + * public/WebMediaPlayerClient.h:
> + * src/WebMediaPlayerClientImpl.cpp:
> + (WebKit::WebMediaPlayerClientImpl::sourceOpened):
> + (WebKit::WebMediaPlayerClientImpl::sourceURL):
> + (WebKit::WebMediaPlayerClientImpl::sourceAppend):
> + (WebKit::WebMediaPlayerClientImpl::sourceEndOfStream):
> + * src/WebMediaPlayerClientImpl.h:
> +
I am going to assume that you have had someone familiar with Chromium's embedder API review these changes, because I am not qualified.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list