[webkit-reviews] review granted: [Bug 195823] Add media stream release logging : [Attachment 364851] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 15 15:28:17 PDT 2019
youenn fablet <youennf at gmail.com> has granted review:
Bug 195823: Add media stream release logging
https://bugs.webkit.org/show_bug.cgi?id=195823
Attachment 364851: Patch
https://bugs.webkit.org/attachment.cgi?id=364851&action=review
--- Comment #4 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 364851
--> https://bugs.webkit.org/attachment.cgi?id=364851
Patch
LGTM.
View in context: https://bugs.webkit.org/attachment.cgi?id=364851&action=review
> Source/WebCore/Modules/mediastream/MediaStream.cpp:311
> LOG(Media, "MediaStream::startProducingData(%p) - not allowed to
start in background, waiting", this);
Move to release logging?
> Source/WebCore/Modules/mediastream/MediaStream.cpp:502
> + ALWAYS_LOG(LOGIDENTIFIER);
Do we need this one, it seems endCaptureTracks) will do these things anyway.
> Source/WebCore/html/HTMLMediaElement.h:564
> + const void* logIdentifier() const final { return m_logIdentifier; }
We could simply return 'this' pointer here and maybe elsewhere.
That would remove the need for m_logIdentifier.
> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:43
> +class MediaStreamTrackPrivate
final here and maybe some other classes.
> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:122
> + const void* logIdentifier() const final { return m_logIdentifier; }
Can these two final be private?
> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:147
> +#endif
This class and some others do not log right now.
Is this code to make it happen in the future or is it allow getting the logger
for some other classes?
> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:176
> + INFO_LOG_IF(loggerPtr(), LOGIDENTIFIER, m_frameCount, " frames
sent in ", delta.value(), " seconds");
Is it safe to log from a background thread?
> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:368
> + return;
Maybe document this change in log, this seems unrelated to adding logging.
> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:272
> + RELEASE_LOG_ERROR(Media, "CoreAudioSharedUnit::setupAudioUnit(%p)
unable to find vpio unit component", this);
Should we go with WebRTC here?
More information about the webkit-reviews
mailing list