Bug 195823 – Add media stream release logging

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Eric Carlson]

Add media stream release logging and update some WebRTC logging.

[Radar WebKit Bug Importer]

<rdar://problem/48939406>

[Eric Carlson]

Created attachment 364851 [details]
Patch

[EWS Watchlist]

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.

[youenn fablet]

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?

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 364867 [details]
Patch for landing.

[youenn fablet]

> >> 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.

[Eric Carlson]

> > >> 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.

[EWS Watchlist]

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.

[Eric Carlson]

Created attachment 364904 [details]
Patch for landing.

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 364904 [details]
Patch for landing.

Clearing flags on attachment: 364904

Committed r243033: <https://trac.webkit.org/changeset/243033>