Bug 146789 – Upload MediaStream Engine to codebase

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Matthew Daiter]

Uploading the source to the codebase is the first step for solving the dependent bug. Not going to link now.

[Radar WebKit Bug Importer]

<rdar://problem/21747025>

[Matthew Daiter]

Created attachment 256513 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:141:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:165:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:535:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:581:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:611:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 5 files


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

[Matthew Daiter]

Created attachment 256514 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:141:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:165:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:535:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:581:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:611:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 5 files


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

[Brent Fulgham]

Comment on attachment 256514 [details]
Patch

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

Looks good. A few things to correct, and we should get Eric's feedback, too.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:93
> +    virtual void cancelLoad() override;

Don't use "virtual" when you use "override". This comment applies for the whole file.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:2
> + * Copyright (C) 2013, 2015 Apple Inc. All rights reserved.

This file is new -- it should only be 2015.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:133
> +    , m_playing(0)

Please use the new C++ initializers in the header file: 
... m_rate { 1 };
... m_seeking { false };
... etc.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:177
> +    && class_getInstanceMethod(getAVSampleBufferAudioRendererClass(), @selector(setMuted:));

This block of "&&"s should be indented one more level.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:180
> +static HashSet<String> mimeTypeCache()

This should be returning a const HashSet<String>&.

I know you were following the code in MediaPlayerPrivateMediaSourceAVFObjc.mm, but the code is wrong there, too.

I'll fix the other case under Bug 146809, but please correct this here.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:182
> +    DEPRECATED_DEFINE_STATIC_LOCAL(HashSet<String>, cache, ());

This should be:

static NeverDestroyed<HashSet<String>> cache();

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:212
> +void MediaPlayerPrivateMediaStreamAVFObjC::load(MediaStreamPrivate* client)

If client can never be null, we should be passing this by reference.  If it can be null, you need to check it.

Judging by our other media code, this is trusted to not be null. But we should require this to be a reference. I filed Bug 146811 to track this.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:248
> +        AVVideoCaptureSource* capture = (AVVideoCaptureSource*)track->source();

Please use a C++-style cast.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:275
> +    

Get rid of this blank line, please.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:307
> +    for (auto it = m_sampleBufferAudioRenderers.begin(), end = m_sampleBufferAudioRenderers.end(); it != end; ++it)

Just use a C++ loop:

for (auto audioRenderer : m_sampleBufferAudioRenderers)
    [*audioRenderer setVolume:volume];

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:319
> +        [*it setMuted:muted];

Ditto the C++ loop comment from above.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:329
> +        AVVideoCaptureSource* source = (AVVideoCaptureSource*)track->source();

C++ cast, please.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:340
> +    return m_MediaStreamPrivate->client()->getVideoTracks().size();

If m_MediaStreamPrivate can never be null, we should store it as a reference. If it can be null, you need to be checking it.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:381
> +    return false;

If it's unseekable, is it really scannable? We say it's scannable above.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:408
> +    return std::make_unique<PlatformTimeRanges>(minMediaTimeSeekable(), maxMediaTimeSeekable());

If it's not seekable, why do we return seemingly-valid times here?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:435
> +    // No-op.

Why is this a no-op? Is this another "// FIXME(125157): Implement painting" case?

If so, you should use "notImplemented();" as well.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:440
> +    // FIXME(125157): Implement painting.

Add "notImplemented();"

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:445
> +    // FIXME(125157): Implement painting.

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaStreamPrivateAVFObjC.mm:50
> +    return mediaStreamPrivate;

Just "return adoptRef(..."

> Source/WebCore/platform/graphics/avfoundation/objc/MediaStreamPrivateAVFObjC.mm:55
> +    , m_client(client)

If these can't be null, they should be references.

[Eric Carlson]

Comment on attachment 256514 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:490
> +    return 0;

The memory cost is clearly not 0. This deserves a FIXME and a bug number.

[Matthew Daiter]

Comment on attachment 256514 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:93
>> +    virtual void cancelLoad() override;
> 
> Don't use "virtual" when you use "override". This comment applies for the whole file.

Sounds good. My bad. Thought the MediaStream class should coordinate with MediaSource.

[Matthew Daiter]

Comment on attachment 256514 [details]
Patch

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

>>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:93
>>> +    virtual void cancelLoad() override;
>> 
>> Don't use "virtual" when you use "override". This comment applies for the whole file.
> 
> Sounds good. My bad. Thought the MediaStream class should coordinate with MediaSource.

Fixed.

[Matthew Daiter]

Comment on attachment 256514 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:133
>> +    , m_playing(0)
> 
> Please use the new C++ initializers in the header file: 
> ... m_rate { 1 };
> ... m_seeking { false };
> ... etc.

Fixed, only for compile-known values

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:177
>> +    && class_getInstanceMethod(getAVSampleBufferAudioRendererClass(), @selector(setMuted:));
> 
> This block of "&&"s should be indented one more level.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:180
>> +static HashSet<String> mimeTypeCache()
> 
> This should be returning a const HashSet<String>&.
> 
> I know you were following the code in MediaPlayerPrivateMediaSourceAVFObjc.mm, but the code is wrong there, too.
> 
> I'll fix the other case under Bug 146809, but please correct this here.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:182
>> +    DEPRECATED_DEFINE_STATIC_LOCAL(HashSet<String>, cache, ());
> 
> This should be:
> 
> static NeverDestroyed<HashSet<String>> cache();

Can't do this inside of this function block. Can't have a function declared in block scope and not in the class have a static storage associated to it

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:212
>> +void MediaPlayerPrivateMediaStreamAVFObjC::load(MediaStreamPrivate* client)
> 
> If client can never be null, we should be passing this by reference.  If it can be null, you need to check it.
> 
> Judging by our other media code, this is trusted to not be null. But we should require this to be a reference. I filed Bug 146811 to track this.

Fixing now. Although this is an override method of MediaPlayerPrivateInterface, which has a LOT of classes inherit it. Do we really want to be modifying all of those base classes?

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:248
>> +        AVVideoCaptureSource* capture = (AVVideoCaptureSource*)track->source();
> 
> Please use a C++-style cast.

Sure.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:275
>> +    
> 
> Get rid of this blank line, please.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:307
>> +    for (auto it = m_sampleBufferAudioRenderers.begin(), end = m_sampleBufferAudioRenderers.end(); it != end; ++it)
> 
> Just use a C++ loop:
> 
> for (auto audioRenderer : m_sampleBufferAudioRenderers)
>     [*audioRenderer setVolume:volume];

Done

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:319
>> +        [*it setMuted:muted];
> 
> Ditto the C++ loop comment from above.

Done

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:329
>> +        AVVideoCaptureSource* source = (AVVideoCaptureSource*)track->source();
> 
> C++ cast, please.

Done

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:340
>> +    return m_MediaStreamPrivate->client()->getVideoTracks().size();
> 
> If m_MediaStreamPrivate can never be null, we should store it as a reference. If it can be null, you need to be checking it.

Just was trying to follow the other class's conventions to make sure we were properly storing. Will change now. Although shouldn't we be storing as a Ref instead then?

[Matthew Daiter]

Comment on attachment 256514 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:340
>> +    return m_MediaStreamPrivate->client()->getVideoTracks().size();
> 
> If m_MediaStreamPrivate can never be null, we should store it as a reference. If it can be null, you need to be checking it.

Can be null. Will check now.

[Matthew Daiter]

Comment on attachment 256514 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:381
>> +    return false;
> 
> If it's unseekable, is it really scannable? We say it's scannable above.

Forgot to change, you are correct. Not scannable.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:435
>> +    // No-op.
> 
> Why is this a no-op? Is this another "// FIXME(125157): Implement painting" case?
> 
> If so, you should use "notImplemented();" as well.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:440
>> +    // FIXME(125157): Implement painting.
> 
> Add "notImplemented();"

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:445
>> +    // FIXME(125157): Implement painting.
> 
> Ditto.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:490
>> +    return 0;
> 
> The memory cost is clearly not 0. This deserves a FIXME and a bug number.

MediaSource was doing this as well...but I'll do that.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaStreamPrivateAVFObjC.mm:50
>> +    return mediaStreamPrivate;
> 
> Just "return adoptRef(..."

Was debugging, my bad.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaStreamPrivateAVFObjC.mm:55
>> +    , m_client(client)
> 
> If these can't be null, they should be references.

MediaSource is doing it. Still change it? Fixed if you want it.

[Matthew Daiter]

Created attachment 256543 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/mediastream/MediaStreamPrivate.h:66:  Too many spaces inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:134:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:158:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:320:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:529:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:575:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:605:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 7 in 6 files


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

[Matthew Daiter]

Comment on attachment 256543 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:175
> +    DEPRECATED_DEFINE_STATIC_LOCAL(HashSet<String>, cache, ());

I would have changed this to a NeverDestroyed, but you can't do that inside of a non-class function according to the compiler.

[Matthew Daiter]

Comment on attachment 256543 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:175
>> +    DEPRECATED_DEFINE_STATIC_LOCAL(HashSet<String>, cache, ());
> 
> I would have changed this to a NeverDestroyed, but you can't do that inside of a non-class function according to the compiler.

Wait...my bad. Yes you can. Compiler was giving me strange errors before.

[Matthew Daiter]

Created attachment 256544 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:134:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:158:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:320:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:529:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:575:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:605:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 6 in 6 files


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

[Brent Fulgham]

Comment on attachment 256544 [details]
Patch

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

Just a couple of small things!

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:336
> +        return 0;

return false;

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:337
> +    return m_MediaStreamPrivate->client()->getVideoTracks().size();

s/size/isEmpty/

!isEmpty

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:343
> +        return 0;

return false!

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:344
> +    return m_MediaStreamPrivate->client()->getAudioTracks().size();

Ditto.

> Source/WebCore/platform/mediastream/MediaStreamPrivate.h:63
> +    MediaStreamPrivate() { }

Make this private so that it's a compile error if you don't pass the parent/client to it.

[Matthew Daiter]

Created attachment 256559 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:134:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:158:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:320:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:529:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:575:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:605:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 6 in 6 files


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

[Eric Carlson]

Comment on attachment 256559 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:77
> +    void seekInternal();
> +    void waitForSeekCompleted();
> +    void seekCompleted();
> +    void setLoadingProgresssed(bool flag) { m_loadingProgressed = flag; }
> +    void setHasAvailableVideoFrame(bool flag) { m_hasAvailableVideoFrame = flag; }
> +    void durationChanged();

These aren't used.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:79
> +    void sizeChanged();

This is unimplemented. Is it needed?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:85
> +        // MediaPlayerPrivateInterface

This class still needs to override a bunch of stuff in HTMLMediaElement, see http://www.w3.org/TR/mediacapture-streams/#media-element-attributes-when-playing-a-mediastream. Please file a bug and add a FIXME to the class somewhere.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:148
> +    // called when the rendering system flips the into or out of accelerated rendering mode.

Typo: "flips the into"

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:164
> +    unsigned long totalVideoFrames() override;
> +    unsigned long droppedVideoFrames() override;
> +    unsigned long corruptedVideoFrames() override;
> +    MediaTime totalFrameDelay() override;

These are only defined in MediaPlayerPrivateInterface for ENABLE(MEDIA_SOURCE)

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:190
> +    bool m_hasAvailableVideoFrame;

This needs to be initialized.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:87
> +@interface AVSampleBufferDisplayLayer (WebCoreAVSampleBufferDisplayLayerPrivate)
> +- (AVVideoPerformanceMetrics *)videoPerformanceMetrics;
> +@end

N

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:96
> +@interface AVSampleBufferAudioRenderer : NSObject
> +- (void)setVolume:(float)volume;
> +- (void)setMuted:(BOOL)muted;
> +@property (nonatomic, copy) NSString *audioTimePitchAlgorithm;
> +@end

Necessary?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:112
> +#pragma mark -
> +#pragma mark AVSampleBufferRenderSynchronizer
> +
> +@interface AVSampleBufferRenderSynchronizer : NSObject
> +- (CMTimebaseRef)timebase;
> +- (float)rate;
> +- (void)setRate:(float)rate;
> +- (void)setRate:(float)rate time:(CMTime)time;
> +- (NSArray *)renderers;
> +- (void)addRenderer:(id)renderer;
> +- (void)removeRenderer:(id)renderer atTime:(CMTime)time withCompletionHandler:(void (^)(BOOL didRemoveRenderer))completionHandler;
> +- (id)addPeriodicTimeObserverForInterval:(CMTime)interval queue:(dispatch_queue_t)queue usingBlock:(void (^)(CMTime time))block;
> +- (id)addBoundaryTimeObserverForTimes:(NSArray *)times queue:(dispatch_queue_t)queue usingBlock:(void (^)(void))block;
> +- (void)removeTimeObserver:(id)observer;
> +@end

Necessary?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:129
> +    , m_synchronizer(adoptNS([allocAVSampleBufferRenderSynchronizerInstance() init]))

This is unused.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:170
> +        && getAVStreamDataParserClass()
> +        && getAVSampleBufferAudioRendererClass()
> +        && getAVSampleBufferRenderSynchronizerClass()
> +        && class_getInstanceMethod(getAVSampleBufferAudioRendererClass(), @selector(setMuted:));

Are these actually necessary?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:216
> +    for (auto track : m_MediaStreamPrivate->client()->getVideoTracks()) {
> +        track->source()->startProducingData();
> +        track->setEnabled(true);
> +    }
> +    for (auto track : m_MediaStreamPrivate->client()->getAudioTracks()) {
> +        track->source()->startProducingData();
> +        track->setEnabled(true);
> +    }
> +    m_player->client().mediaPlayerRenderingModeChanged(m_player);

The spec says you also need to create audio and video tracks:

For each MediaStreamTrack in the MediaStream , including those that are added after the User Agent enters the media element load algorithm, the User Agent must create a corresponding AudioTrack or VideoTrack as defined in [HTML5].

Please file a bug and add a FIXME for this.

It also says that only MediaStreamTracks with a readyState of 'live' should be enabled:

Let the media resource, represented by the MediaStream object, indicate to the media element load algorithm that all audio tracks and all live video tracks (represented by a MediaStreamTrack with the readyState attribute set to live) should be enabled. This allows the media element load algorithm to set AudioTrack.enabled, VideoTrack.selected and VideoTrackList.selectedIndex accordingly.

http://www.w3.org/TR/mediacapture-streams/#loading-and-playing-a-mediastream-in-a-media-element

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:223
> +void MediaPlayerPrivateMediaStreamAVFObjC::prepareToPlay()

Nit: This isn't pure virtual in MediaPlayerPrivateInterface so you don't need to override.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:240
> +        // Just grab the first webcam for now, we can manage more later

Please add a FIXME and bug number.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:295
> +void MediaPlayerPrivateMediaStreamAVFObjC::pause()
> +{
> +    auto weakThis = createWeakPtr();
> +    callOnMainThread([weakThis] {
> +        if (!weakThis)
> +            return;
> +        weakThis.get()->pauseInternal();
> +    });
> +}
> +
> +void MediaPlayerPrivateMediaStreamAVFObjC::pauseInternal()
> +{
> +    m_playing = false;
> +    [m_synchronizer setRate:0];
> +    if (m_MediaStreamPrivate) {
> +        for (auto track : m_MediaStreamPrivate->client()->getVideoTracks())
> +            track->source()->stopProducingData();
> +        for (auto track : m_MediaStreamPrivate->client()->getAudioTracks())
> +            track->source()->stopProducingData();
> +    }
> +}
> +
> +bool MediaPlayerPrivateMediaStreamAVFObjC::paused() const
> +{
> +    return ![m_synchronizer rate];
> +}

Shouldn't "paused" be passed through to the MediaStream?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:301
> +void MediaPlayerPrivateMediaStreamAVFObjC::setVolume(float volume)
> +{
> +    for (auto audioRenderer : m_sampleBufferAudioRenderers)
> +        [audioRenderer setVolume:volume];
> +}

You don't use an audio renderer (yet?). Please remove until it is needed.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:311
> +    for (auto audioRenderer : m_sampleBufferAudioRenderers)
> +        [audioRenderer setMuted:muted];

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:327
> +            if (source->width() > floatSize.width())
> +                floatSize.setWidth(source->width());
> +            if (source->height() > floatSize.height())
> +                floatSize.setHeight(source->height());

If a track isn't visible/enabled, does it still return its intrinsic width & height? If so, you should skip non-visible tracks.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:363
> +    if (synchronizerTime < m_lastSeekTime)
> +        return m_lastSeekTime;

Seeking isn't possible.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:375
> +MediaTime MediaPlayerPrivateMediaStreamAVFObjC::startTime() const
> +{
> +    return MediaTime::zeroTime();
> +}
> +
> +MediaTime MediaPlayerPrivateMediaStreamAVFObjC::initialTime() const
> +{
> +    return MediaTime::zeroTime();
> +}

No need to override, MediaPlayerPrivate returns zeroTime.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:393
> +void MediaPlayerPrivateMediaStreamAVFObjC::setPreservesPitch(bool preservesPitch)
> +{
> +    NSString *algorithm = preservesPitch ? AVAudioTimePitchAlgorithmSpectral : AVAudioTimePitchAlgorithmVarispeed;
> +    for (auto& renderer : m_sampleBufferAudioRenderers)
> +        [renderer setAudioTimePitchAlgorithm:algorithm];
> +}

A MediaStream is not seekable, so "preservesPitch" doesn't make sense. This can be removed.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:505
> +unsigned long MediaPlayerPrivateMediaStreamAVFObjC::totalVideoFrames()
> +{
> +    return [[m_sampleBufferDisplayLayer videoPerformanceMetrics] totalNumberOfVideoFrames];
> +}
> +
> +unsigned long MediaPlayerPrivateMediaStreamAVFObjC::droppedVideoFrames()
> +{
> +    return [[m_sampleBufferDisplayLayer videoPerformanceMetrics] numberOfDroppedVideoFrames];
> +}
> +
> +unsigned long MediaPlayerPrivateMediaStreamAVFObjC::corruptedVideoFrames()
> +{
> +    return [[m_sampleBufferDisplayLayer videoPerformanceMetrics] numberOfCorruptedVideoFrames];
> +}
> +
> +MediaTime MediaPlayerPrivateMediaStreamAVFObjC::totalFrameDelay()
> +{
> +    return MediaTime::createWithDouble([[m_sampleBufferDisplayLayer videoPerformanceMetrics] totalFrameDelay]);
> +}

Not useful until there is a sampleBufferDisplayLayer.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:518
> +    if (m_sampleBufferDisplayLayer)
> +        return;
> +
> +    m_sampleBufferDisplayLayer = adoptNS([allocAVSampleBufferDisplayLayerInstance() init]);
> +#ifndef NDEBUG
> +    [m_sampleBufferDisplayLayer setName:@"MediaPlayerPrivateMediaStream AVSampleBufferDisplayLayer"];
> +#endif

platformLayer() returns a AVCaptureVideoPreviewLayer. Is this necessary?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:532
> +    if (!m_sampleBufferDisplayLayer)
> +        return;
> +
> +    CMTime currentTime = CMTimebaseGetTime([m_synchronizer timebase]);
> +    [m_synchronizer removeRenderer:m_sampleBufferDisplayLayer.get() atTime:currentTime withCompletionHandler:^(BOOL) {
> +        // No-op.
> +    }];
> +    m_sampleBufferDisplayLayer = nullptr;

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:555
> +void MediaPlayerPrivateMediaStreamAVFObjC::addDisplayLayer(AVSampleBufferDisplayLayer* displayLayer)

It doesn't look like this is used (yet?). If it isn't, you should remove it until it is actually needed.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:569
> +void MediaPlayerPrivateMediaStreamAVFObjC::removeDisplayLayer(AVSampleBufferDisplayLayer* displayLayer)

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:583
> +void MediaPlayerPrivateMediaStreamAVFObjC::addAudioRenderer(AVSampleBufferAudioRenderer* audioRenderer)

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:598
> +void MediaPlayerPrivateMediaStreamAVFObjC::removeAudioRenderer(AVSampleBufferAudioRenderer* audioRenderer)

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:613
> +void MediaPlayerPrivateMediaStreamAVFObjC::characteristicsChanged()

Ditto.

[Matthew Daiter]

Comment on attachment 256559 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:77
>> +    void durationChanged();
> 
> These aren't used.

Sure. Removed and fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:79
>> +    void sizeChanged();
> 
> This is unimplemented. Is it needed?

Yep! Fixed. Sent callback to sizeChanged inside of m_player.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:85
>> +        // MediaPlayerPrivateInterface
> 
> This class still needs to override a bunch of stuff in HTMLMediaElement, see http://www.w3.org/TR/mediacapture-streams/#media-element-attributes-when-playing-a-mediastream. Please file a bug and add a FIXME to the class somewhere.

Which class?

[Matthew Daiter]

Comment on attachment 256559 [details]
Patch

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

>>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:85
>>> +        // MediaPlayerPrivateInterface
>> 
>> This class still needs to override a bunch of stuff in HTMLMediaElement, see http://www.w3.org/TR/mediacapture-streams/#media-element-attributes-when-playing-a-mediastream. Please file a bug and add a FIXME to the class somewhere.
> 
> Which class?

Oh! Got it. Fixed, have it in another bug.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:148
>> +    // called when the rendering system flips the into or out of accelerated rendering mode.
> 
> Typo: "flips the into"

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:164
>> +    MediaTime totalFrameDelay() override;
> 
> These are only defined in MediaPlayerPrivateInterface for ENABLE(MEDIA_SOURCE)

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:190
>> +    bool m_hasAvailableVideoFrame;
> 
> This needs to be initialized.

Doesn't seem to be used. Removing.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:87
>> +@end
> 
> N

Left these in due to planning ahead, in case we have to make a video and audio renderer and synchronizer for WebRTC. In the case that we don't want this code lying around before that happens, I can remove this.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:96
>> +@end
> 
> Necessary?

Ditto.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:112
>> +@end
> 
> Necessary?

Ditto.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:129
>> +    , m_synchronizer(adoptNS([allocAVSampleBufferRenderSynchronizerInstance() init]))
> 
> This is unused.

Ditto.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:170
>> +        && class_getInstanceMethod(getAVSampleBufferAudioRendererClass(), @selector(setMuted:));
> 
> Are these actually necessary?

Ditto. And CoreMedia is certainly used! Need it for the AVVideoCapturePreviewLayer interpretation.

[Matthew Daiter]

Created attachment 256623 [details]
Patch

[Matthew Daiter]

Comment on attachment 256559 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:216
>> +    m_player->client().mediaPlayerRenderingModeChanged(m_player);
> 
> The spec says you also need to create audio and video tracks:
> 
> For each MediaStreamTrack in the MediaStream , including those that are added after the User Agent enters the media element load algorithm, the User Agent must create a corresponding AudioTrack or VideoTrack as defined in [HTML5].
> 
> Please file a bug and add a FIXME for this.
> 
> It also says that only MediaStreamTracks with a readyState of 'live' should be enabled:
> 
> Let the media resource, represented by the MediaStream object, indicate to the media element load algorithm that all audio tracks and all live video tracks (represented by a MediaStreamTrack with the readyState attribute set to live) should be enabled. This allows the media element load algorithm to set AudioTrack.enabled, VideoTrack.selected and VideoTrackList.selectedIndex accordingly.
> 
> http://www.w3.org/TR/mediacapture-streams/#loading-and-playing-a-mediastream-in-a-media-element

Done!

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:223
>> +void MediaPlayerPrivateMediaStreamAVFObjC::prepareToPlay()
> 
> Nit: This isn't pure virtual in MediaPlayerPrivateInterface so you don't need to override.

Compiler is throwing errors about not overriding

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:240
>> +        // Just grab the first webcam for now, we can manage more later
> 
> Please add a FIXME and bug number.

Done!

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:301
>> +}
> 
> You don't use an audio renderer (yet?). Please remove until it is needed.

Fixed

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:311
>> +        [audioRenderer setMuted:muted];
> 
> Ditto.

Fixed

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:327
>> +                floatSize.setHeight(source->height());
> 
> If a track isn't visible/enabled, does it still return its intrinsic width & height? If so, you should skip non-visible tracks.

Fixed

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:375
>> +}
> 
> No need to override, MediaPlayerPrivate returns zeroTime.

Fixed

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:393
>> +}
> 
> A MediaStream is not seekable, so "preservesPitch" doesn't make sense. This can be removed.

Fixed

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:518
>> +#endif
> 
> platformLayer() returns a AVCaptureVideoPreviewLayer. Is this necessary?

Fixed

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:532
>> +    m_sampleBufferDisplayLayer = nullptr;
> 
> Ditto.

Fixed

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:555
>> +void MediaPlayerPrivateMediaStreamAVFObjC::addDisplayLayer(AVSampleBufferDisplayLayer* displayLayer)
> 
> It doesn't look like this is used (yet?). If it isn't, you should remove it until it is actually needed.

Fixed

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:569
>> +void MediaPlayerPrivateMediaStreamAVFObjC::removeDisplayLayer(AVSampleBufferDisplayLayer* displayLayer)
> 
> Ditto.

Fixed

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:92:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 6 files


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

[Brent Fulgham]

Comment on attachment 256623 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:265
> +    return m_MediaStreamPrivate->client()->getVideoTracks().size();

I think these should use "return !m_MediaStreamPrivate... getVideoTracks().isEmpty();"

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:270
> +    return m_MediaStreamPrivate->client()->getAudioTracks().size();

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaStreamPrivateAVFObjC.mm:59
> +

Please get rid of this blank line.

[Matthew Daiter]

Created attachment 256627 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:92:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 6 files


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

[Brent Fulgham]

Comment on attachment 256627 [details]
Patch

r=me. Nice work!

[WebKit Commit Bot]

Comment on attachment 256627 [details]
Patch

Clearing flags on attachment: 256627

Committed r186766: <http://trac.webkit.org/changeset/186766>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.