Add media stream release logging and update some WebRTC logging.
<rdar://problem/48939406>
Created attachment 364851 [details] Patch
Attachment 364851 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/IntSize.h:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:1085: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:188: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:189: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:190: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:191: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.cpp:176: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 7 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 364851 [details] 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?
Comment on attachment 364851 [details] Patch 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? Oops, thanks. >> Source/WebCore/Modules/mediastream/MediaStream.cpp:502 >> + ALWAYS_LOG(LOGIDENTIFIER); > > Do we need this one, it seems endCaptureTracks) will do these things anyway. OK >> 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. We aren't supposed to log addresses. >> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:43 >> +class MediaStreamTrackPrivate > > final here and maybe some other classes. OK >> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:122 >> + const void* logIdentifier() const final { return m_logIdentifier; } > > Can these two final be private? They are both used by the RealtimeOutgoing sources. >> 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? It sets the RealtimeMediaSource logger/log identifier. >> 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? No, but this is always called on the main thread. >> 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? Good idea, changed.
Created attachment 364867 [details] Patch for landing.
> >> 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? > > No, but this is always called on the main thread. I am not sure we always log from the main thread, videoSampleAvailable might be called from the capture thread. This might be a preexisting issue in fact.
> > >> 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? > > > > No, but this is always called on the main thread. > > I am not sure we always log from the main thread, videoSampleAvailable might > be called from the capture thread. This might be a preexisting issue in fact. As we discussed on irc, this should only be called on the main thread. I'll add an ASSERT in a new patch.
Attachment 364867 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/IntSize.h:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:1085: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:188: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:189: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:190: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:191: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.cpp:176: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 7 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 364904 [details] Patch for landing.
Attachment 364904 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/IntSize.h:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:1085: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:188: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:189: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:190: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:191: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.cpp:176: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 7 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 364904 [details] Patch for landing. Clearing flags on attachment: 364904 Committed r243033: <https://trac.webkit.org/changeset/243033>