Bug 194348 – [MSE] Convert debug-only logging to runtime logging

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Convert debug-only logging to runtime logging

[Eric Carlson]

<rdar://problem/47566449>

[Eric Carlson]

Created attachment 361338 [details]
Patch

[EWS Watchlist]

Attachment 361338 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.cpp:231:  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/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:98:  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/Modules/mediasource/MediaSource.cpp:62:  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/Modules/mediasource/MediaSource.cpp:76:  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: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 361338 [details]
Patch

Attachment 361338 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11059077

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 361352 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Eric Carlson]

Created attachment 361396 [details]
Patch

[EWS Watchlist]

Attachment 361396 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.cpp:231:  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/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:98:  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/Modules/mediasource/MediaSource.cpp:61:  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/Modules/mediasource/MediaSource.cpp:75:  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: 4 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 361396 [details]
Patch

Attachment 361396 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11067015

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 361407 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 361396 [details]
Patch

Attachment 361396 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11067106

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 361410 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 361396 [details]
Patch

Attachment 361396 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11067185

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 361413 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Eric Carlson]

Created attachment 361415 [details]
Patch

[EWS Watchlist]

Attachment 361415 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.cpp:231:  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/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:98:  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/Modules/mediasource/MediaSource.cpp:61:  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/Modules/mediasource/MediaSource.cpp:75:  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: 4 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 361415 [details]
Patch

Clearing flags on attachment: 361415

Committed r241148: <https://trac.webkit.org/changeset/241148>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

Broke the GTK build: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Build%29/builds/18845

In file included from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource-4babe430-14.cpp:3:0:
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp: In member function ‘virtual void WebCore::SourceBuffer::sourceBufferPrivateDidReceiveRenderingError(int)’:
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp:719:68: warning: unused parameter ‘error’ [-Wunused-parameter]
 void SourceBuffer::sourceBufferPrivateDidReceiveRenderingError(int error)
                                                                    ^~~~~
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp: In function ‘WebCore::PlatformTimeRanges WebCore::removeSamplesFromTrackBuffer(const MapType&, WebCore::SourceBuffer::TrackBuffer&, const WebCore::SourceBuffer*, const char*)’:
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp:742:65: error: ‘const class WebCore::SourceBuffer’ has no member named ‘logClassName’
     auto logIdentifier = WTF::Logger::LogSiteIdentifier(buffer->logClassName(), logPrefix, buffer->logIdentifier());
                                                                 ^~~~~~~~~~~~
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp:742:100: error: ‘const class WebCore::SourceBuffer’ has no member named ‘logIdentifier’
     auto logIdentifier = WTF::Logger::LogSiteIdentifier(buffer->logClassName(), logPrefix, buffer->logIdentifier());
                                                                                                    ^~~~~~~~~~~~~
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp:743:28: error: ‘const class WebCore::SourceBuffer’ has no member named ‘logger’
     auto& logger = buffer->logger();
                            ^~~~~~
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp:744:43: error: ‘const class WebCore::SourceBuffer’ has no member named ‘logChannel’
     auto willLog = logger.willLog(buffer->logChannel(), WTFLogLevelDebug);
                                           ^~~~~~~~~~
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp:761:34: error: ‘const class WebCore::SourceBuffer’ has no member named ‘logChannel’
             logger.debug(buffer->logChannel(), logIdentifier, "removing sample ", *sampleIt.second);
                                  ^~~~~~~~~~
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp:813:30: error: ‘const class WebCore::SourceBuffer’ has no member named ‘logChannel’
         logger.debug(buffer->logChannel(), logIdentifier, "removed ", bytesRemoved, ", start = ", earliestSample, ", end = ", latestSample);
                              ^~~~~~~~~~
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp: In member function ‘void WebCore::SourceBuffer::evictCodedFrames(size_t)’:
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp:961:12: warning: unused variable ‘initialBufferedSize’ [-Wunused-variable]
     size_t initialBufferedSize = extraMemoryCost();
            ^~~~~~~~~~~~~~~~~~~

[Philippe Normand]

Comment on attachment 361415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361415&action=review

> Source/WebCore/Modules/mediasource/MediaSource.h:117
> +#if !RELEASE_LOG_DISABLED

Seems like this should be !LOG_DISABLED. The inconsistency might have spread elsewhere in the patch.

[Philippe Normand]

I thought it might be simpler to change the call sites: https://trac.webkit.org/r241251

Please follow-up if this was wrong, I didn't want to roll-out the patch.

[Eric Carlson]

(In reply to Philippe Normand from comment #20)
> I thought it might be simpler to change the call sites:
> https://trac.webkit.org/r241251
> 
> Please follow-up if this was wrong, I didn't want to roll-out the patch.

This is correct, thank you for fixing my mistake!

[Michael Catanzaro]

More:

[2392/3510] Building CXX object Source...ources/UnifiedSource-4babe430-14.cpp.o
In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-4babe430-14.cpp:3:
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp: In member function ‘virtual void WebCore::SourceBuffer::sourceBufferPrivateDidReceiveRenderingError(int)’:
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp:719:68: warning: unused parameter ‘error’ [-Wunused-parameter]
 void SourceBuffer::sourceBufferPrivateDidReceiveRenderingError(int error)
                                                                ~~~~^~~~~
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp: In member function ‘void WebCore::SourceBuffer::evictCodedFrames(size_t)’:
../../Source/WebCore/Modules/mediasource/SourceBuffer.cpp:961:12: warning: unused variable ‘initialBufferedSize’ [-Wunused-variable]
     size_t initialBufferedSize = extraMemoryCost();
            ^~~~~~~~~~~~~~~~~~~

It's quite confusing, but DEBUG_LOG() is still release logging so we need to use RELEASE_LOG_DISABLED here too.

(I'm not sure why, but RELEASE_LOG_DISABLED is defined when building with 'build-webkit --gtk --debug'. That seems surprising.)

[Michael Catanzaro]

Committed r241472: <https://trac.webkit.org/changeset/241472>

[Michael Catanzaro]

Ah, well I learned something today. RELEASE_LOG() is Mac-specific, added in https://trac.webkit.org/changeset/197214/webkit (and later renamed). So the MSE logging is now disabled on other platforms.

I'd wondered before why RELEASE_LOG() didn't seem to work. Huh. I guess that's intentional, so OK?