[webkit-reviews] review granted: [Bug 177869] Make RealtimeIncomingVideoSources and RealtimeOutgoingVideoSources port agnostic : [Attachment 322810] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 5 03:58:09 PDT 2017


youenn fablet <youennf at gmail.com> has granted  review:
Bug 177869: Make RealtimeIncomingVideoSources and RealtimeOutgoingVideoSources
port agnostic
https://bugs.webkit.org/show_bug.cgi?id=177869

Attachment 322810: Patch

https://bugs.webkit.org/attachment.cgi?id=322810&action=review




--- Comment #17 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 322810
  --> https://bugs.webkit.org/attachment.cgi?id=322810
Patch

r=me provided that bots are happy.
Some comments below to improve the code since we are touching it.

Thinking about it, I guess RealtimeXXMac.h/.cpp should probably named
RealtimeXXCocoa.h/.cpp

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

> Source/WebCore/platform/mediastream/RealtimeIncomingVideoSource.cpp:47
> +    auto source =
RealtimeIncomingVideoSourceMac::create(WTFMove(videoTrack), WTFMove(trackId));

We can probably move RealtimeIncomingVideoSource::create implementation to
RealtimeIncomingVideoSourceMac.cpp.
That would remove the FIXME and the need to include
RealtimeIncomingVideoSourceMac.h here.
We duplicate source->start() but this is probably ok.

> Source/WebCore/platform/mediastream/RealtimeIncomingVideoSource.h:67
> +    void OnFrame(const webrtc::VideoFrame&) override { };

Can we remove this one since GTK is not compiling with LIBWEBRTC on currently?

> Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:49
> +    return RealtimeOutgoingVideoSourceMac::create(WTFMove(videoSource));

Ditto here for moving this to RealtimeOutgoingVideoSourceMac.cpp.

> Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:87
> +    bool GetStats(Stats*) final;

Could be inlined since it is a one liner function.

> Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:99
> +    virtual void videoSampleAvailable(MediaSample&) { };

Since videoSampleAvailable would be made virtual, maybe it is best to kill it
and implement the already virtual sampleBufferUpdated directly in the port
specific classes.

>
Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceMac.cpp:149
> +    callOnMainThread([protectedThis = WTFMove(protectedThis), sample =
WTFMove(sample), width, height, rotation] {

protectedThis = makeRef(*this)

> Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceMac.h:16
> + *	 software without specific prior written permission.

Remove this third clause here and in other Mac.h/.cpp files since they are new.

> Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSourceMac.h:40
> +    ~RealtimeOutgoingVideoSourceMac() { }

Probably not needed.


More information about the webkit-reviews mailing list