Bug 194348 - [MSE] Convert debug-only logging to runtime logging
Summary: [MSE] Convert debug-only logging to runtime logging
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-06 10:38 PST by Eric Carlson
Modified: 2019-02-13 14:30 PST (History)
8 users (show)

See Also:


Attachments
Patch (109.79 KB, patch)
2019-02-06 15:37 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-highsierra (2.72 MB, application/zip)
2019-02-06 17:58 PST, Build Bot
no flags Details
Patch (112.04 KB, patch)
2019-02-07 08:27 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.96 MB, application/zip)
2019-02-07 09:55 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (3.02 MB, application/zip)
2019-02-07 10:14 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (2.59 MB, application/zip)
2019-02-07 10:38 PST, Build Bot
no flags Details
Patch (114.81 KB, patch)
2019-02-07 10:50 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2019-02-06 10:38:11 PST
Convert debug-only logging to runtime logging
Comment 1 Eric Carlson 2019-02-06 10:50:47 PST
<rdar://problem/47566449>
Comment 2 Eric Carlson 2019-02-06 15:37:57 PST
Created attachment 361338 [details]
Patch
Comment 3 Build Bot 2019-02-06 15:40:58 PST
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.
Comment 4 Build Bot 2019-02-06 17:58:28 PST
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.
Comment 5 Build Bot 2019-02-06 17:58:31 PST
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
Comment 6 Eric Carlson 2019-02-07 08:27:13 PST
Created attachment 361396 [details]
Patch
Comment 7 Build Bot 2019-02-07 08:30:16 PST
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.
Comment 8 Build Bot 2019-02-07 09:55:10 PST
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.
Comment 9 Build Bot 2019-02-07 09:55:12 PST
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
Comment 10 Build Bot 2019-02-07 10:14:16 PST
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.
Comment 11 Build Bot 2019-02-07 10:14:17 PST
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
Comment 12 Build Bot 2019-02-07 10:38:42 PST
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.
Comment 13 Build Bot 2019-02-07 10:38:44 PST
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
Comment 14 Eric Carlson 2019-02-07 10:50:48 PST
Created attachment 361415 [details]
Patch
Comment 15 Build Bot 2019-02-07 10:53:46 PST
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.
Comment 16 WebKit Commit Bot 2019-02-07 14:58:16 PST
Comment on attachment 361415 [details]
Patch

Clearing flags on attachment: 361415

Committed r241148: <https://trac.webkit.org/changeset/241148>
Comment 17 WebKit Commit Bot 2019-02-07 14:58:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Philippe Normand 2019-02-10 11:17:58 PST
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();
            ^~~~~~~~~~~~~~~~~~~
Comment 19 Philippe Normand 2019-02-10 11:20:20 PST
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.
Comment 20 Philippe Normand 2019-02-10 11:35:48 PST
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.
Comment 21 Eric Carlson 2019-02-11 06:40:16 PST
(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!
Comment 22 Michael Catanzaro 2019-02-13 14:21:28 PST
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.)
Comment 23 Michael Catanzaro 2019-02-13 14:23:56 PST
Committed r241472: <https://trac.webkit.org/changeset/241472>
Comment 24 Michael Catanzaro 2019-02-13 14:30:30 PST
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?