Bug 91773

Summary: create a MediaSource object
Product: WebKit Reporter: Anna Cavender <annacc>
Component: MediaAssignee: 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: P2    
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

Description Anna Cavender 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
Comment 1 Anna Cavender 2012-07-24 17:10:51 PDT
Created attachment 154181 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 WebKit Review Bot 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
Comment 4 Anna Cavender 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.
Comment 5 Anna Cavender 2012-08-05 18:22:55 PDT
Created attachment 156579 [details]
Patch
Comment 6 Gyuyoung Kim 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.
Comment 7 Anna Cavender 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!
Comment 8 Anna Cavender 2012-08-05 20:49:05 PDT
Created attachment 156590 [details]
fixing CMakeLists.txt
Comment 9 Gyuyoung Kim 2012-08-06 08:33:40 PDT
(In reply to comment #7)
> Sure thing, thanks for catching that!

Thanks, looks fine in CMake.
Comment 10 Eric Carlson 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.
Comment 11 Anna Cavender 2012-08-06 10:28:52 PDT
Committed r124780: <http://trac.webkit.org/changeset/124780>
Comment 12 WebKit Review Bot 2012-08-06 13:21:05 PDT
Re-opened since this is blocked by 93294
Comment 13 Anna Cavender 2012-08-06 14:41:07 PDT
r124798 fixed the Android build problem caused by this patch.
Comment 14 WebKit Review Bot 2012-08-06 23:21:53 PDT
Re-opened since this is blocked by 93340
Comment 15 Anna Cavender 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.
Comment 16 Anna Cavender 2012-08-07 14:20:33 PDT
*** Bug 93285 has been marked as a duplicate of this bug. ***
Comment 17 Eric Carlson 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.
Comment 18 Anna Cavender 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.
Comment 19 Eric Carlson 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.
Comment 20 Anna Cavender 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.
Comment 21 WebKit Review Bot 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>
Comment 22 WebKit Review Bot 2012-08-07 17:46:51 PDT
All reviewed patches have been landed.  Closing bug.