Bug 91773

Summary:

create a MediaSource object

Product:

WebKit

Reporter:

Anna Cavender <annacc>

Component:

Media

Assignee:

Anna Cavender <annacc>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, acolwell, dglazkov, eric.carlson, feature-media-reviews, gyuyoung.kim, ojan, rakuco, vestbo, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

93294, 93340

Bug Blocks:

90819

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from gce-cr-linux-08	none
Patch	none
fixing CMakeLists.txt	none
Patch	none

Anna Cavender

Reported 2012-07-19 12:40:34 PDT

A MediaSource object is needed in order to implement the new object-oriented MediaSource API: http://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html#mediasource

Attachments
Patch (43.47 KB, patch) 2012-07-24 17:10 PDT, Anna Cavender	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-08 (1.02 MB, application/zip) 2012-07-24 20:37 PDT, WebKit Review Bot	no flags	Details
Patch (43.45 KB, patch) 2012-08-05 18:22 PDT, Anna Cavender	no flags	Details Formatted Diff Diff
fixing CMakeLists.txt (43.52 KB, patch) 2012-08-05 20:49 PDT, Anna Cavender	no flags	Details Formatted Diff Diff
Patch (46.58 KB, patch) 2012-08-07 14:18 PDT, Anna Cavender	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Anna Cavender

Comment 1 2012-07-24 17:10:51 PDT

Created attachment 154181 [details] Patch

WebKit Review Bot

Comment 2 2012-07-24 20:37:53 PDT

Comment on attachment 154181 [details] Patch Attachment 154181 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13350072 New failing tests: fast/loader/loadInProgress.html fast/inline/positionedLifetime.html fast/loader/unload-form-post-about-blank.html

WebKit Review Bot

Comment 3 2012-07-24 20:37:59 PDT

Created attachment 154234 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Anna Cavender

Comment 4 2012-08-05 09:40:16 PDT

Ping! Pretty sure those test failures aren't related. I'll upload a fresh patch this afternoon to be sure. But this should be ready for review as is.

Anna Cavender

Comment 5 2012-08-05 18:22:55 PDT

Created attachment 156579 [details] Patch

Gyuyoung Kim

Comment 6 2012-08-05 20:28:08 PDT

Comment on attachment 156579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156579&action=review > Source/WebCore/CMakeLists.txt:2667 > + Modules/mediasource/MediaSource.idl Could you move this to existing file list? Because, r124730 removed ENALBE_MEDIA_SOURCE in CMakeLists.txt > Source/WebCore/CMakeLists.txt:2672 > + Modules/mediasource/MediaSource.cpp ditto.

Anna Cavender

Comment 7 2012-08-05 20:45:29 PDT

Comment on attachment 156579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156579&action=review >> Source/WebCore/CMakeLists.txt:2667 >> + Modules/mediasource/MediaSource.idl > > Could you move this to existing file list? Because, r124730 removed ENALBE_MEDIA_SOURCE in CMakeLists.txt Sure thing, thanks for catching that!

Anna Cavender

Comment 8 2012-08-05 20:49:05 PDT

Created attachment 156590 [details] fixing CMakeLists.txt

Gyuyoung Kim

Comment 9 2012-08-06 08:33:40 PDT

(In reply to comment #7) > Sure thing, thanks for catching that! Thanks, looks fine in CMake.

Eric Carlson

Comment 10 2012-08-06 09:40:28 PDT

Comment on attachment 156590 [details] fixing CMakeLists.txt View in context: https://bugs.webkit.org/attachment.cgi?id=156590&action=review > Source/WebCore/Modules/mediasource/MediaSource.cpp:97 > + RefPtr<SourceBuffer> buffer = SourceBuffer::create(String::number(++m_nextSourceBufferId), this); Does this need to deal with m_nextSourceBufferId wrapping around? > Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:44 > + ASSERT(isMainThread()); Why is this ASSERT necessary? A comment would be helpful.

Anna Cavender

Comment 11 2012-08-06 10:28:52 PDT

Committed r124780: <http://trac.webkit.org/changeset/124780>

WebKit Review Bot

Comment 12 2012-08-06 13:21:05 PDT

Re-opened since this is blocked by 93294

Anna Cavender

Comment 13 2012-08-06 14:41:07 PDT

r124798 fixed the Android build problem caused by this patch.

WebKit Review Bot

Comment 14 2012-08-06 23:21:53 PDT

Re-opened since this is blocked by 93340

Anna Cavender

Comment 15 2012-08-07 14:18:19 PDT

Created attachment 157002 [details] Patch Lets try that again. This patch also includes the addition of a sourceURLs list in PublicURLManager to avoid the problems encounted with the last patch.

Anna Cavender

Comment 16 2012-08-07 14:20:33 PDT

*** Bug 93285 has been marked as a duplicate of this bug. ***

Eric Carlson

Comment 17 2012-08-07 14:56:10 PDT

(In reply to comment #10) > (From update of attachment 156590 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156590&action=review > > > Source/WebCore/Modules/mediasource/MediaSource.cpp:97 > > + RefPtr<SourceBuffer> buffer = SourceBuffer::create(String::number(++m_nextSourceBufferId), this); > > Does this need to deal with m_nextSourceBufferId wrapping around? > You forgot to comment on this question.

Anna Cavender

Comment 18 2012-08-07 15:09:47 PDT

Comment on attachment 156590 [details] fixing CMakeLists.txt View in context: https://bugs.webkit.org/attachment.cgi?id=156590&action=review Oops, forgot to click Publish. >>> Source/WebCore/Modules/mediasource/MediaSource.cpp:97 >>> + RefPtr<SourceBuffer> buffer = SourceBuffer::create(String::number(++m_nextSourceBufferId), this); >> >> Does this need to deal with m_nextSourceBufferId wrapping around? > > You forgot to comment on this question. Yes. We will handle this on the chromium side by checking for duplicate source ids and throwing a ReachedIdLimit. >> Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:44 >> + ASSERT(isMainThread()); > > Why is this ASSERT necessary? A comment would be helpful. Done.

Eric Carlson

Comment 19 2012-08-07 15:16:51 PDT

(In reply to comment #18) > (From update of attachment 156590 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156590&action=review > > Oops, forgot to click Publish. > > >>> Source/WebCore/Modules/mediasource/MediaSource.cpp:97 > >>> + RefPtr<SourceBuffer> buffer = SourceBuffer::create(String::number(++m_nextSourceBufferId), this); > >> > >> Does this need to deal with m_nextSourceBufferId wrapping around? > > > > You forgot to comment on this question. > > Yes. We will handle this on the chromium side by checking for duplicate source ids and throwing a ReachedIdLimit. > Why do it in platform specific code? This will require every port that implements the feature to duplicate the check.

Anna Cavender

Comment 20 2012-08-07 15:57:42 PDT

(In reply to comment #19) > (In reply to comment #18) > > (From update of attachment 156590 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=156590&action=review > > > > Oops, forgot to click Publish. > > > > >>> Source/WebCore/Modules/mediasource/MediaSource.cpp:97 > > >>> + RefPtr<SourceBuffer> buffer = SourceBuffer::create(String::number(++m_nextSourceBufferId), this); > > >> > > >> Does this need to deal with m_nextSourceBufferId wrapping around? > > > > > > You forgot to comment on this question. > > > > Yes. We will handle this on the chromium side by checking for duplicate source ids and throwing a ReachedIdLimit. > > > Why do it in platform specific code? This will require every port that implements the feature to duplicate the check. Good point. As per our offline discussion, I will file a bug to address this. Will probably use size_t for the type of m_nextSourceBufferId and cap the number of SourceBuffers that may be added at the size of the sourceBuffers vector.

WebKit Review Bot

Comment 21 2012-08-07 17:46:43 PDT

Comment on attachment 157002 [details] Patch Clearing flags on attachment: 157002 Committed r124953: <http://trac.webkit.org/changeset/124953>

WebKit Review Bot

Comment 22 2012-08-07 17:46:51 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.