Bug 146461 - Implement platform specific part of enumerateDevices
Summary: Implement platform specific part of enumerateDevices
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: Matthew Daiter
URL:
Keywords: HTML5, InRadar
Depends on:
Blocks: 146426
  Show dependency treegraph
 
Reported: 2015-06-30 10:50 PDT by Matthew Daiter
Modified: 2015-07-09 13:48 PDT (History)
7 users (show)

See Also:


Attachments
Patch (15.16 KB, patch)
2015-06-30 18:37 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff
Patch (15.28 KB, patch)
2015-07-01 11:01 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff
Patch (17.48 KB, patch)
2015-07-01 11:20 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff
Patch (15.85 KB, patch)
2015-07-01 11:22 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff
Patch (16.88 KB, patch)
2015-07-01 11:40 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff
Patch (18.01 KB, patch)
2015-07-01 11:52 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff
Patch (17.68 KB, patch)
2015-07-01 11:53 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff
Patch (17.03 KB, patch)
2015-07-01 11:54 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff
Patch (16.86 KB, patch)
2015-07-01 17:11 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff
Patch (15.02 KB, patch)
2015-07-02 11:19 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff
Patch (17.24 KB, patch)
2015-07-09 11:49 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff
Patch (17.13 KB, patch)
2015-07-09 12:08 PDT, Matthew Daiter
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Daiter 2015-06-30 10:50:25 PDT
Need to get platform specific code in for enumerateDevices. Should be the first code committed: everything depends on it and nothing is necessary to implement.
Comment 1 Radar WebKit Bug Importer 2015-06-30 10:56:29 PDT
<rdar://problem/21614466>
Comment 2 Matthew Daiter 2015-06-30 18:37:22 PDT
Created attachment 255887 [details]
Patch
Comment 3 Eric Carlson 2015-07-01 10:05:25 PDT
Comment on attachment 255887 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDevicesPrivate.cpp:49
> +    Vector<RefPtr<TrackSourceInfo>> capturedDevices = AVCaptureDeviceManager::singleton().getSourcesInfo("");

Nit: I think you mean "captureDevices", not "capturedDevices".

> Source/WebCore/Modules/mediastream/MediaDevicesPrivate.cpp:57
> +            RefPtr<MediaDeviceInfo> mediaDeviceInfo = MediaDeviceInfo::create(context, capturedDevices[i].get()->label(), capturedDevices[i].get()->deviceId(), capturedDevices[i].get()->groupId(), "audioinput");
> +            mediaDevicesInfo.append(mediaDeviceInfo);
> +        } else {
> +            RefPtr<MediaDeviceInfo> mediaDeviceInfo = MediaDeviceInfo::create(context, capturedDevices[i].get()->label(), capturedDevices[i].get()->deviceId(), capturedDevices[i].get()->groupId(), "videoinput");
> +            mediaDevicesInfo.append(mediaDeviceInfo);

1) you should put capturedDevices[i].get() into a local instead of calling it four times.

2) the MediaDeviceKind string values should be constants exported by the MediaDeviceInfo class

3) this would be easier to understand if you rewrote it to get rid of the "else"

   TrackSourceInfo* trackInfo = capturedDevices[i].get();
   String deviceType = trackInfo->kind() == TrackSourceInfo::SourceKind::Audio ? MediaDeviceInfo::audioInputType() : MediaDeviceInfo::videoInputType();
   mediaDevicesInfo.append(MediaDeviceInfo::create(context, trackInfo->label(), trackInfo->deviceId(), trackInfo->groupId(), deviceType));

> Source/WebCore/Modules/mediastream/MediaDevicesPrivate.h:39
> +

Nit: don't need this extra blank line here.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:342
> +            sourcesInfo.append(TrackSourceInfo::create(devices[i].m_videoSourceId, TrackSourceInfo::Video, device.localizedName, device.localizedName, devices[i].m_captureDeviceID));
>          if (devices[i].m_audioSource)
> -            sourcesInfo.append(TrackSourceInfo::create(devices[i].m_audioSourceId, TrackSourceInfo::Audio, device.localizedName));
> +            sourcesInfo.append(TrackSourceInfo::create(devices[i].m_audioSourceId, TrackSourceInfo::Audio, device.localizedName, device.localizedName, devices[i].m_captureDeviceID));

It is probably not always correct to use the localized name for the groupID. I don't know what we should use for group ID, but please add a FIXME here and we can talk about this.
Comment 4 Matthew Daiter 2015-07-01 11:01:49 PDT
Created attachment 255932 [details]
Patch
Comment 5 Matthew Daiter 2015-07-01 11:20:22 PDT
Created attachment 255934 [details]
Patch
Comment 6 Matthew Daiter 2015-07-01 11:22:57 PDT
Created attachment 255935 [details]
Patch
Comment 7 Eric Carlson 2015-07-01 11:28:30 PDT
Comment on attachment 255935 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.h:52
> +    static const String audioInputType() { return "audioinput"; }
> +    static const String audioOutputType() { return "audiooutput"; }
> +    static const String videoInputType() { return "videoinput"; }

These create a new String each time they are called. It would be much more efficient to create and return a static AtomicString reference, see TextTrack::subtitlesKeyword for example.
Comment 8 Matthew Daiter 2015-07-01 11:40:43 PDT
Created attachment 255937 [details]
Patch
Comment 9 Brent Fulgham 2015-07-01 11:47:23 PDT
Comment on attachment 255937 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.cpp:52
> +    DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, audioinput, ("audioinput", AtomicString::ConstructFromLiteral));

We aren't supposed to use DEPRECATED_DEFINE_STATIC_LOCAL anymore. I think you want the NeverDestroy<> template. Talk to andersca if you want more details.

> Source/WebCore/Modules/mediastream/MediaDevicesPrivate.cpp:51
> +    for (size_t i = 0; i < capturedDevices.size(); i++) {

This should be a modern C++ loop:

for (auto& trackInfo : capturedDevices) ....
Comment 10 Brent Fulgham 2015-07-01 11:48:12 PDT
Comment on attachment 255937 [details]
Patch

Looks great. Get rid of the deprecated DEPRECATED_DEFINE_STATIC_LOCAL and switch to a modern C++ iterator, please.
Comment 11 Matthew Daiter 2015-07-01 11:52:24 PDT
Created attachment 255939 [details]
Patch
Comment 12 Matthew Daiter 2015-07-01 11:53:33 PDT
Created attachment 255940 [details]
Patch
Comment 13 Matthew Daiter 2015-07-01 11:54:38 PDT
Created attachment 255941 [details]
Patch
Comment 14 Matthew Daiter 2015-07-01 17:11:43 PDT
Created attachment 255973 [details]
Patch
Comment 15 Matthew Daiter 2015-07-02 11:19:01 PDT
Created attachment 256020 [details]
Patch
Comment 16 Darin Adler 2015-07-02 15:02:00 PDT
Comment on attachment 256020 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.cpp:34
>  #include "ScriptWrappable.h"
>  
> +#include <wtf/NeverDestroyed.h>

No blank line here in WebKit coding style.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.cpp:44
> +    return adoptRef(*new MediaDevicesPrivate());

I normally leave off the parentheses when there are no arguments like this.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:35
> +#include "MediaDeviceInfo.h"
> +
> +#include <wtf/RefCounted.h>

No blank spaces needed for this.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:36
> +#include <wtf/RefPtr.h>

Should probably include <wtf/Forward.h> instead since we don’t actually need to work with a RefPtr.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:41
> +class MediaDevicesPrivate : public RefCounted<MediaDevicesPrivate> {

Why does this need to be reference counted? Is there more than one owner for this? Since we need polymorphism but not sharing, then please use std::unique_ptr for this. Lets not reference count things unless there is actually some benefit.

I’m concerned that there are other classes already in the media stream implementation that are also unnecessarily reference counted.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:45
> +    static RefPtr<MediaDevicesPrivate> create();

Return type should be Ref, not RefPtr.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:49
> +    virtual Vector<RefPtr<MediaDeviceInfo>> availableMediaDevices(ScriptExecutionContext*);

Argument type should be Document& or, if this is being made to work with worker threads, then perhaps ScriptExecutionContext&.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:56
> +#endif /* MediaDevicesPrivate_h */

We don’t use /* */ for this in WebKit coding style.

> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:46
> +    static PassRefPtr<TrackSourceInfo> create(const AtomicString& id, SourceKind kind, const AtomicString& label, const AtomicString& groupId, const AtomicString& deviceId)

Return type should be Ref, not PassRefPtr. Please see <http://www.webkit.org/coding/RefPtr.html>.

> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:72
> +    TrackSourceInfo(const AtomicString& id, SourceKind kind, const AtomicString& label, const AtomicString& groupId, const AtomicString& deviceId)
> +        : m_groupId(groupId)
> +        , m_deviceId(deviceId)
> +        , m_id(id)
> +        , m_kind(kind)
> +        , m_label(label)
> +    {
> +    }

Strange that the order in the constructor doesn’t match the order of the data members. Really we want a single order.
Comment 17 Matthew Daiter 2015-07-09 11:39:51 PDT
Comment on attachment 256020 [details]
Patch

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

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:35
>> +#include <wtf/RefCounted.h>
> 
> No blank spaces needed for this.

Fixed

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:36
>> +#include <wtf/RefPtr.h>
> 
> Should probably include <wtf/Forward.h> instead since we don’t actually need to work with a RefPtr.

Fixed

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:41
>> +class MediaDevicesPrivate : public RefCounted<MediaDevicesPrivate> {
> 
> Why does this need to be reference counted? Is there more than one owner for this? Since we need polymorphism but not sharing, then please use std::unique_ptr for this. Lets not reference count things unless there is actually some benefit.
> 
> I’m concerned that there are other classes already in the media stream implementation that are also unnecessarily reference counted.

Just thought it'd conform to how the rest of the codebase was laid out. Fixed.

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:45
>> +    static RefPtr<MediaDevicesPrivate> create();
> 
> Return type should be Ref, not RefPtr.

Done

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:49
>> +    virtual Vector<RefPtr<MediaDeviceInfo>> availableMediaDevices(ScriptExecutionContext*);
> 
> Argument type should be Document& or, if this is being made to work with worker threads, then perhaps ScriptExecutionContext&.

Changed to ScriptExecutionContext&

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:56
>> +#endif /* MediaDevicesPrivate_h */
> 
> We don’t use /* */ for this in WebKit coding style.

Fixed

>> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:46
>> +    static PassRefPtr<TrackSourceInfo> create(const AtomicString& id, SourceKind kind, const AtomicString& label, const AtomicString& groupId, const AtomicString& deviceId)
> 
> Return type should be Ref, not PassRefPtr. Please see <http://www.webkit.org/coding/RefPtr.html>.

Fixed

>> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:72
>> +    }
> 
> Strange that the order in the constructor doesn’t match the order of the data members. Really we want a single order.

Fixed
Comment 18 Matthew Daiter 2015-07-09 11:49:18 PDT
Created attachment 256497 [details]
Patch
Comment 19 Matthew Daiter 2015-07-09 12:08:22 PDT
Created attachment 256499 [details]
Patch
Comment 20 Brent Fulgham 2015-07-09 12:56:14 PDT
Comment on attachment 256499 [details]
Patch

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

r=me

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.cpp:47
> +        TrackSourceInfo* trackInfo = device.get();

Do you really need this temporary? I would have expected "trackInfo->kind()" and "device->kind()" to be the same thing.
Comment 21 WebKit Commit Bot 2015-07-09 13:48:22 PDT
Comment on attachment 256499 [details]
Patch

Clearing flags on attachment: 256499

Committed r186608: <http://trac.webkit.org/changeset/186608>
Comment 22 WebKit Commit Bot 2015-07-09 13:48:25 PDT
All reviewed patches have been landed.  Closing bug.