Bug 91773 – create a MediaSource object

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

[Anna Cavender]

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

[Anna Cavender]

Created attachment 154181 [details]
Patch

[WebKit Review Bot]

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]

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]

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]

Created attachment 156579 [details]
Patch

[Gyuyoung Kim]

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

Created attachment 156590 [details]
fixing CMakeLists.txt

[Gyuyoung Kim]

(In reply to comment #7)
> Sure thing, thanks for catching that!

Thanks, looks fine in CMake.

[Eric Carlson]

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]

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

[WebKit Review Bot]

Re-opened since this is blocked by 93294

[Anna Cavender]

r124798 fixed the Android build problem caused by this patch.

[WebKit Review Bot]

Re-opened since this is blocked by 93340

[Anna Cavender]

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]

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

[Eric Carlson]

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

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

(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 on attachment 157002 [details]
Patch

Clearing flags on attachment: 157002

Committed r124953: <http://trac.webkit.org/changeset/124953>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.