Bug 150997 – [MediaStream] Add mock audio and video sources

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Create mock realtime audio and video sources for testing.

[Radar WebKit Bug Importer]

<rdar://problem/23453358>

[Eric Carlson]

Created attachment 265028 [details]
Patch for the bots to chew on.

[WebKit Commit Bot]

Attachment 265028 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:96:  Missing space before ( in while(  [whitespace/parens] [5]
ERROR: Source/WebCore/platform/mock/MockRealtimeMediaSource.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 265029 [details]
Updated patch.

[WebKit Commit Bot]

Attachment 265029 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mock/MockRealtimeMediaSource.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 265030 [details]
Updated patch.

[WebKit Commit Bot]

Attachment 265030 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:39:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:41:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:53:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:56:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 4 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 265043 [details]
Another updated patch.

[Jer Noble]

Comment on attachment 265043 [details]
Another updated patch.

r=me, with nits.

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:-363
> -#ifndef NDEBUG
>      m_videoBackgroundLayer.get().name = @"MediaPlayerPrivateMediaStreamAVFObjC preview background layer";
> -#endif

Is there a reason you removed this #ifdef? I don't think we generally set the layers' names in release builds.

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:63
> +    m_previewLayer.get().name = @"MockRealtimeVideoSourceMac preview layer";

And I think this may need a #ifdef as well.

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:69
> +#if !PLATFORM(IOS)
> +    m_previewLayer.get().autoresizingMask = kCALayerWidthSizable | kCALayerHeightSizable;
> +#endif

You said this wasn't necessary in MediaPlayerPrivateMediaStreamAVFObjC::createPreviewLayers().  Is it necessary here?

> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:78
> +void MockRealtimeVideoSourceMac::updatePlatformLayer() const
> +{
> +#if TARGET_OS_IPHONE || (PLATFORM(MAC))

Is there a reason why this entire file shouldn't be wrapped this way?  Also, I think that you may want PLATFORM(COCOA) here.

[Eric Carlson]

Comment on attachment 265043 [details]
Another updated patch.

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:-363
>> -#endif
> 
> Is there a reason you removed this #ifdef? I don't think we generally set the layers' names in release builds.

Simon says (see what I did there?) it is fine to set layer names in a release build as long as the name doesn't have to be computed.

>> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:63
>> +    m_previewLayer.get().name = @"MockRealtimeVideoSourceMac preview layer";
> 
> And I think this may need a #ifdef as well.

Ditto.

>> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:69
>> +#endif
> 
> You said this wasn't necessary in MediaPlayerPrivateMediaStreamAVFObjC::createPreviewLayers().  Is it necessary here?

Because this layer is set as a sub-layer of the layer returned by MediaPlayerPrivateMediaStreamAVFObjC.

>> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:78
>> +#if TARGET_OS_IPHONE || (PLATFORM(MAC))
> 
> Is there a reason why this entire file shouldn't be wrapped this way?  Also, I think that you may want PLATFORM(COCOA) here.

Oops!

[Eric Carlson]

Created attachment 265085 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 265085 [details]
Patch for landing.

Clearing flags on attachment: 265085

Committed r192174: <http://trac.webkit.org/changeset/192174>