Bug 122358 - Create MediaStream object with ended attribute set if all tracks that are being used on its creation are ended
Summary: Create MediaStream object with ended attribute set if all tracks that are bei...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago de Barros Lacerda
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-04 16:29 PDT by Thiago de Barros Lacerda
Modified: 2013-10-10 16:15 PDT (History)
9 users (show)

See Also:


Attachments
Patch (13.12 KB, patch)
2013-10-10 07:48 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (13.11 KB, patch)
2013-10-10 13:03 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (13.12 KB, patch)
2013-10-10 13:21 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (13.12 KB, patch)
2013-10-10 13:27 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (13.43 KB, patch)
2013-10-10 15:39 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago de Barros Lacerda 2013-10-04 16:29:20 PDT
Spec tells that if all tracks that belongs to a new MediaStream object being created are ended, then the ended attribute of this MediaStream must be set to true.
http://www.w3.org/TR/mediacapture-streams/#widl-MediaStream-ended
Comment 1 Thiago de Barros Lacerda 2013-10-10 07:48:15 PDT
Created attachment 213882 [details]
Patch
Comment 2 Thiago de Barros Lacerda 2013-10-10 13:03:39 PDT
Created attachment 213917 [details]
Patch
Comment 3 Thiago de Barros Lacerda 2013-10-10 13:21:50 PDT
Created attachment 213920 [details]
Patch
Comment 4 Thiago de Barros Lacerda 2013-10-10 13:27:08 PDT
Created attachment 213921 [details]
Patch
Comment 5 Jer Noble 2013-10-10 14:11:45 PDT
Comment on attachment 213921 [details]
Patch

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

A few nits:

> Source/WebCore/Modules/mediastream/MediaStream.cpp:104
> -    for (size_t i = 0; i < tracks.size(); ++i)
> +    bool ended = true;
> +    for (size_t i = 0; i < tracks.size(); ++i) {
> +        ended &= tracks[i]->ended();
>          processTrack(tracks[i].get(), tracks[i]->kind() == "audio" ? audioSources : videoSources);
> +    }
>  
> -    return createFromSourceVectors(context, audioSources, videoSources);
> +    return createFromSourceVectors(context, audioSources, videoSources, ended);

Nit: I would have named this "allEnded" just to make clear what you're doing with this variable.

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:55
> -    static PassRefPtr<MediaStreamDescriptor> create(const MediaStreamSourceVector& audioSources, const MediaStreamSourceVector& videoSources);
> +    static PassRefPtr<MediaStreamDescriptor> create(const MediaStreamSourceVector& audioSources, const MediaStreamSourceVector& videoSources, bool ended);

I don't particularly like adding new "bool" parameters to creation functions.  It's hard to figure out what that true or false parameter means from the a call site.  The preferred way to do this is with an enum:

enum EndedAtCreationFlags { IsNotEnded, IsEnded };
static PassRefPtr<MediaStream> createFromSourceVectors(ScriptExecutionContext* context, const MediaStreamSourceVector& audioSources, const MediaStreamSourceVector& videoSources, EndedAtCreationFlags flags)

Then the EndedAtCreationFlags type would be used elsewhere where this method (or similar ones) are called.

> Source/WebCore/platform/mock/MockMediaStreamCenter.cpp:236
> -    client->didCreateStream(MediaStreamDescriptor::create(audioSources, videoSources));
> +    client->didCreateStream(MediaStreamDescriptor::create(audioSources, videoSources, false));

...Then this line becomes much more understandable:

client->didCreateStream(MediaStreamDescriptor::create(audioSources, videoSources, MediaStreamDescriptor::IsNotEnded));

There are a few examples like this in HTMLMediaElement.h
Comment 6 Thiago de Barros Lacerda 2013-10-10 15:39:32 PDT
Created attachment 213936 [details]
Patch
Comment 7 Thiago de Barros Lacerda 2013-10-10 15:40:28 PDT
Adding Enum on MediaStreamDescriptor create method
Comment 8 Jer Noble 2013-10-10 15:42:53 PDT
Comment on attachment 213936 [details]
Patch

Looks good! r=me.
Comment 9 WebKit Commit Bot 2013-10-10 16:15:47 PDT
Comment on attachment 213936 [details]
Patch

Clearing flags on attachment: 213936

Committed r157268: <http://trac.webkit.org/changeset/157268>
Comment 10 WebKit Commit Bot 2013-10-10 16:15:50 PDT
All reviewed patches have been landed.  Closing bug.