[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