[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