Bug 76614

Summary:

MediaStream API: Split the MediaStream track list into audio/video specific ones.

Product:

WebKit

Reporter:

Tommy Widenflycht <tommyw>

Component:

WebKit Misc.

Assignee:

Tommy Widenflycht <tommyw>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dglazkov, fishd, harald, ojan, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

56459

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Tommy Widenflycht

Reported 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.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Tommy Widenflycht

Comment 1 2012-01-19 02:34:26 PST

Created attachment 123091 [details] Patch

WebKit Review Bot

Comment 2 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.

WebKit Review Bot

Comment 3 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

Tommy Widenflycht

Comment 4 2012-01-19 04:49:35 PST

Created attachment 123108 [details] Patch

Darin Fisher (:fishd, Google)

Comment 5 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.

Tommy Widenflycht

Comment 6 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.

Tommy Widenflycht

Comment 7 2012-01-23 04:31:50 PST

Created attachment 123539 [details] Patch

WebKit Review Bot

Comment 8 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>

WebKit Review Bot

Comment 9 2012-01-24 12:30:09 PST

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.