Bug 76614 - MediaStream API: Split the MediaStream track list into audio/video specific ones.
Summary: MediaStream API: Split the MediaStream track list into audio/video specific o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 56459
  Show dependency treegraph
 
Reported: 2012-01-19 02:29 PST by Tommy Widenflycht
Modified: 2012-01-24 12:30 PST (History)
6 users (show)

See Also:


Attachments
Patch (34.62 KB, patch)
2012-01-19 02:34 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (34.90 KB, patch)
2012-01-19 04:49 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (34.62 KB, patch)
2012-01-23 04:31 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2012-01-19 02:29:29 PST
The latest draft of the WebRTC standard have split the MediaStream combined track list into audio/video specific ones.
Comment 1 Tommy Widenflycht 2012-01-19 02:34:26 PST
Created attachment 123091 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-19 02:37:08 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 WebKit Review Bot 2012-01-19 03:36:31 PST
Comment on attachment 123091 [details]
Patch

Attachment 123091 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11299055

New failing tests:
platform/chromium/media/video-capture-preview.html
Comment 4 Tommy Widenflycht 2012-01-19 04:49:35 PST
Created attachment 123108 [details]
Patch
Comment 5 Darin Fisher (:fishd, Google) 2012-01-20 10:58:50 PST
Comment on attachment 123108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123108&action=review

> Source/WebCore/mediastream/MediaStream.cpp:41
> +void processTrackList(PassRefPtr<MediaStreamTrackList> prpTracks, String kind, MediaStreamSourceVector& sources, ExceptionCode& ec)

nit: this function should be marked static or placed in an anonymous namespace.
nit: String should be passed as |const String&|

> Source/WebCore/mediastream/MediaStream.cpp:44
> +    if (tracks.get()) {

nit: consider an early return if !tracks.get()

what is the reason for storing prpTracks as tracks?

> Source/WebKit/chromium/public/WebUserMediaClient.h:46
> +    virtual void requestUserMedia(const WebUserMediaRequest&, const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&) { }

nit: add parameter names for the new function?  it isn't always obvious which is the set of the audio tracks and which is the set of video tracks.

shouldn't the comment just be about declaring the first function as DEPRECATED?  the second function is the new one, right?

> Source/WebKit/chromium/public/WebUserMediaRequest.h:74
> +    WEBKIT_EXPORT void requestSucceeded(const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&);

same nit:  please name the parameters.  it is not clear otherwise which one is for audio and which one is for video.

> Source/WebKit/chromium/public/platform/WebMediaStreamDescriptor.h:59
> +    WEBKIT_EXPORT void initialize(const WebString& label, const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&);

same nit.
Comment 6 Tommy Widenflycht 2012-01-23 04:27:19 PST
Comment on attachment 123108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123108&action=review

>> Source/WebCore/mediastream/MediaStream.cpp:41
>> +void processTrackList(PassRefPtr<MediaStreamTrackList> prpTracks, String kind, MediaStreamSourceVector& sources, ExceptionCode& ec)
> 
> nit: this function should be marked static or placed in an anonymous namespace.
> nit: String should be passed as |const String&|

Fixed * 2

>> Source/WebCore/mediastream/MediaStream.cpp:44
>> +    if (tracks.get()) {
> 
> nit: consider an early return if !tracks.get()
> 
> what is the reason for storing prpTracks as tracks?

Fixed nit.

Don't think I understand your question. I need to store the PassRefPtr into a RefPtr so I can use it more than once.

>> Source/WebKit/chromium/public/WebUserMediaClient.h:46
>> +    virtual void requestUserMedia(const WebUserMediaRequest&, const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&) { }
> 
> nit: add parameter names for the new function?  it isn't always obvious which is the set of the audio tracks and which is the set of video tracks.
> 
> shouldn't the comment just be about declaring the first function as DEPRECATED?  the second function is the new one, right?

Fixed

>> Source/WebKit/chromium/public/WebUserMediaRequest.h:74
>> +    WEBKIT_EXPORT void requestSucceeded(const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&);
> 
> same nit:  please name the parameters.  it is not clear otherwise which one is for audio and which one is for video.

Fixed.

>> Source/WebKit/chromium/public/platform/WebMediaStreamDescriptor.h:59
>> +    WEBKIT_EXPORT void initialize(const WebString& label, const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&);
> 
> same nit.

Fixed.
Comment 7 Tommy Widenflycht 2012-01-23 04:31:50 PST
Created attachment 123539 [details]
Patch
Comment 8 WebKit Review Bot 2012-01-24 12:30:02 PST
Comment on attachment 123539 [details]
Patch

Clearing flags on attachment: 123539

Committed r105774: <http://trac.webkit.org/changeset/105774>
Comment 9 WebKit Review Bot 2012-01-24 12:30:09 PST
All reviewed patches have been landed.  Closing bug.