Bug 132558 - [MediaStream] MediaStream.addTrack Should not check for active state.
Summary: [MediaStream] MediaStream.addTrack Should not check for active state.
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-04 22:07 PDT by Kiran
Modified: 2014-05-13 06:56 PDT (History)
12 users (show)

See Also:


Attachments
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. (1.39 KB, patch)
2014-05-04 22:59 PDT, Kiran
eric.carlson: review-
Details | Formatted Diff | Diff
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. (8.04 KB, patch)
2014-05-08 00:14 PDT, Kiran
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (511.28 KB, application/zip)
2014-05-08 01:10 PDT, Build Bot
no flags Details
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. (8.04 KB, patch)
2014-05-08 03:06 PDT, Kiran
eric.carlson: review-
Details | Formatted Diff | Diff
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. (7.63 KB, patch)
2014-05-08 21:52 PDT, Kiran
eric.carlson: review+
eric.carlson: commit-queue-
Details | Formatted Diff | Diff
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. (7.56 KB, patch)
2014-05-09 21:45 PDT, Kiran
commit-queue: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track. (7.56 KB, patch)
2014-05-09 21:51 PDT, Kiran
kiran.guduru: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kiran 2014-05-04 22:07:50 PDT
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state.
The following use case will not work, if it checks for active state while adding a new track.

1. Create a MediaStream without any argument, then try to add tracks to mediaStream will not work.
Comment 1 Kiran 2014-05-04 22:59:08 PDT
Created attachment 230808 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

MediaStream.addTrack Should not check for active state.
Comment 2 Eric Carlson 2014-05-05 08:39:59 PDT
(In reply to comment #1)
> Created an attachment (id=230808) [details]
> MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.
> 
> MediaStream.addTrack Should not check for active state.

What, specifically, does the spec say about this?
Comment 3 Kiran 2014-05-05 21:21:30 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=230808) [details] [details]
> > MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.
> > 
> > MediaStream.addTrack Should not check for active state.
> 
> What, specifically, does the spec say about this?

This bug arise while modifying MediaStream.ended state to active state.
Spec is not specifying to check for active state in addTrack steps [1].



1.Let track be the MediaStreamTrack argument and stream this MediaStream object.
2.If track is already in stream's track set, then abort these steps.
3.Add track to stream's track set.

Further if we add this check then the scenario mentioned in comment 1, i.e., 
"Create a MediaStream without any argument, then try to add tracks to the newly created mediaStream" will not work, because in this case since a newly created mediaStream has no tracks its state is inactive.

[1] http://dev.w3.org/2011/webrtc/editor/getusermedia.html#widl-MediaStream-addTrack-void-MediaStreamTrack-track
Comment 4 Eric Carlson 2014-05-06 10:34:53 PDT
Comment on attachment 230808 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

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

> Source/WebCore/ChangeLog:11
> +        No new tests, no behavior change.

This is incorrect, you note the need for a behavior change in the bug: "Create a MediaStream without any argument, then try to add tracks to the newly created mediaStream". 

It sounds like it should be easy to create a test case for this change.
Comment 5 Kiran 2014-05-08 00:14:09 PDT
Created attachment 231049 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

The new patch file is modified according to the comments received.
Please review the new patch file.
Comment 6 Build Bot 2014-05-08 01:10:05 PDT
Comment on attachment 231049 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

Attachment 231049 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6166218729848832

New failing tests:
svg/text/non-bmp-positioning-lists.svg
media/W3C/video/networkState/networkState_during_loadstart.html
Comment 7 Build Bot 2014-05-08 01:10:10 PDT
Created attachment 231052 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 8 Kiran 2014-05-08 03:06:41 PDT
Created attachment 231057 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

Modified patch.
Comment 9 Eric Carlson 2014-05-08 07:19:50 PDT
Comment on attachment 231057 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

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

r- for now because the test needs a little bit of work, but this is close. Thanks for responding to previous feedback.

> LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:9
> +    <body>
> +        <p id="description"></p>
> +        <div id="console"></div>
> +        <script>

I know that you based this on an existing test, but I would really prefer to have this <script> element inside of <head> and call it from body.load or some such.

> LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:17
> +            function error() {

Nit: WebKit coding style is to put a function's opening brace on a new line.

> LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:40
> +            function shouldNotFire() {

This is not used. You need to add "onaddtrack" and "onremovetrack" handlers to stream2.

> LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:51
> +            function shouldFireActive() {
> +                debug("Stream2 is active.");
> +                finishJSTest();
> +            }
> +
> +            function shouldFireInActive() {
> +                debug("Stream2 is inactive.");
> +                finishJSTest();

Neither of these is called, should they be? If not, each should call testFailed(...) instead of debug() to make it clear that there is an error.

> LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:54
> +            function createStreamAndAddTRacks() {

Nit: "TRacks" should be "Tracks"
Comment 10 Kiran 2014-05-08 21:52:59 PDT
Created attachment 231129 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

Modified the Test file as per the comments.
Comment 11 Eric Carlson 2014-05-09 09:30:24 PDT
Comment on attachment 231129 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

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

I am marking this r+ but not cq+ because of the minor changes needed. That means that your revised patch does not need to be reviewed again, so when you upload a new patch:

1 -put "Reviewed by Eric Carlson" in the ChangeLog files
2 - name your patch "Patch for landing"
3 - DO NOT mark this patch as obsolete
4 - mark the new patch cq+ but not r? (because it doesn't need a new review)

and any reviewer can mark it cq+.

Thanks for sticking with this!

> LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:6
> +        <p id="description"></p>
> +        <div id="console"></div>

Oops, these elements should be in <body> intend of in <head>!

> LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:8
> +            description("Test adding tracks to inactive MediaStream.");

This can go in startMedia().

> LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:57
> +            //getUserMedia({audio:true, video:true}, gotStream1);

Nit: this should be removed.

> LayoutTests/fast/mediastream/MediaStream-add-tracks-to-inactive-stream.html:68
> +    <body onload="javascript:startMedia()">

Nit: "javascript:" is unnecessary.
Comment 12 Kiran 2014-05-09 21:45:56 PDT
Created attachment 231211 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

Thanks for review.
The patch is modified and attached as per the review comments.
Comment 13 WebKit Commit Bot 2014-05-09 21:46:23 PDT
Comment on attachment 231211 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

Rejecting attachment 231211 [details] from review queue.

kiran.guduru@samsung.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.
Comment 14 WebKit Commit Bot 2014-05-09 21:47:00 PDT
Comment on attachment 231211 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

Rejecting attachment 231211 [details] from commit-queue.

kiran.guduru@samsung.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 15 Kiran 2014-05-09 21:51:59 PDT
Created attachment 231212 [details]
MediaStream.addTrack method is checking for active state of a MediaStream, but it should not check for active state while adding a Track.

Thanks for reviewing.
Plese commit the modified patch as per review comments.
Comment 16 Alberto Garcia 2014-05-13 06:56:54 PDT
Committed r168679: <http://trac.webkit.org/changeset/168679>