Bug 150997 - [MediaStream] Add mock audio and video sources
Summary: [MediaStream] Add mock audio and video sources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-11-07 08:15 PST by Eric Carlson
Modified: 2016-01-04 13:14 PST (History)
4 users (show)

See Also:


Attachments
Patch for the bots to chew on. (70.66 KB, patch)
2015-11-08 18:59 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch. (70.66 KB, patch)
2015-11-08 19:54 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch. (73.76 KB, patch)
2015-11-08 20:01 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Another updated patch. (70.66 KB, patch)
2015-11-09 05:54 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing. (70.56 KB, patch)
2015-11-09 13:02 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2015-11-07 08:15:35 PST
Create mock realtime audio and video sources for testing.
Comment 1 Radar WebKit Bug Importer 2015-11-07 08:16:24 PST
<rdar://problem/23453358>
Comment 2 Eric Carlson 2015-11-08 18:59:45 PST
Created attachment 265028 [details]
Patch for the bots to chew on.
Comment 3 WebKit Commit Bot 2015-11-08 19:01:56 PST
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.
Comment 4 Eric Carlson 2015-11-08 19:54:15 PST
Created attachment 265029 [details]
Updated patch.
Comment 5 WebKit Commit Bot 2015-11-08 19:56:09 PST
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.
Comment 6 Eric Carlson 2015-11-08 20:01:29 PST
Created attachment 265030 [details]
Updated patch.
Comment 7 WebKit Commit Bot 2015-11-08 20:03:15 PST
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.
Comment 8 Eric Carlson 2015-11-09 05:54:34 PST
Created attachment 265043 [details]
Another updated patch.
Comment 9 Jer Noble 2015-11-09 09:38:09 PST
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.
Comment 10 Eric Carlson 2015-11-09 13:01:21 PST
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!
Comment 11 Eric Carlson 2015-11-09 13:02:00 PST
Created attachment 265085 [details]
Patch for landing.
Comment 12 WebKit Commit Bot 2015-11-09 13:20:07 PST
Comment on attachment 265085 [details]
Patch for landing.

Clearing flags on attachment: 265085

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