Bug 220423

Summary: [Mac] Add runtime logging to format reader and WebM parser
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, cdumez, darin, esprehn+autocc, ews-watchlist, glenn, jer.noble, kangil.han, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing
none
Address review comments none

Description Eric Carlson 2021-01-07 11:12:39 PST
Add runtime logging to format reader and WebM parser to make failures easier to diagnose
Comment 1 Radar WebKit Bug Importer 2021-01-07 11:12:47 PST
<rdar://problem/72896655>
Comment 2 Eric Carlson 2021-01-07 13:40:31 PST
Created attachment 417208 [details]
Patch
Comment 3 Andy Estes 2021-01-07 14:13:18 PST
Comment on attachment 417208 [details]
Patch

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

> Source/WebKit/Shared/mac/MediaFormatReader/MediaTrackReader.cpp:78
> +    , m_logIdentifier(formatReader.nextTrackReaderLogIdentifier())

Can trackID also be the log identifier here?
Comment 4 Andy Estes 2021-01-07 14:16:44 PST
Comment on attachment 417208 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:538
> +static RefPtr<Logger>& staticSharedLogger()
> +{
> +    static NeverDestroyed<RefPtr<Logger>> logger;
> +    return logger;
> +}
> +
> +RefPtr<Logger>& Document::sharedLogger()
> +{
> +    if (!staticSharedLogger().get()) {
> +        staticSharedLogger() = Logger::create(sharedLoggerOwner());
> +        configureSharedLogger();
> +    }
> +    
> +    return staticSharedLogger();
> +}

Nit: I think `static Logger*` would work just as well as `static NeverDestroyed<RefPtr<Logger>>`. This Logger lives forever, so you could just `.leakRef()` the result of `Logger::create()` when initializing `staticSharedLogger()`.
Comment 5 Eric Carlson 2021-01-07 15:10:24 PST
Comment on attachment 417208 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:538
>> +}
> 
> Nit: I think `static Logger*` would work just as well as `static NeverDestroyed<RefPtr<Logger>>`. This Logger lives forever, so you could just `.leakRef()` the result of `Logger::create()` when initializing `staticSharedLogger()`.

Good idea. I changed it and made `Document::sharedLogger` to return a `const Logger&`

>> Source/WebKit/Shared/mac/MediaFormatReader/MediaTrackReader.cpp:78
>> +    , m_logIdentifier(formatReader.nextTrackReaderLogIdentifier())
> 
> Can trackID also be the log identifier here?

Yes, I updated `MediaFormatReader.nextTrackReaderLogIdentifier` to take the track ID to use as the child portion of the identifier.
Comment 6 Eric Carlson 2021-01-07 15:15:40 PST
Created attachment 417216 [details]
Patch for landing
Comment 7 EWS 2021-01-07 16:18:37 PST
Committed r271270: <https://trac.webkit.org/changeset/271270>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417216 [details].
Comment 8 Darin Adler 2021-01-08 10:36:17 PST
Comment on attachment 417216 [details]
Patch for landing

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

Why are we using "const void*" for identifiers instead of something that doesn't require reinterpret_cast. Is there some reason pretending this integer is a pointer has value?

> Source/WebCore/dom/Document.cpp:549
> +    bool alwaysOnLoggingAllowed = !allDocumentsMap().isEmpty() && WTF::allOf(allDocumentsMap().values(), [](auto* document) {
> +        auto* page = document->page();
> +        return !page || page->sessionID().isAlwaysOnLoggingAllowed();
> +    });

Is the emptiness check valuable? I’d expect allOf/values to efficiently do nothing for an empty map without an explicit check.

> Source/WebCore/dom/Document.cpp:643
>      ASSERT_UNUSED(addResult, addResult.isNewEntry);

Seems like we should move this assertion into Document::addToDocumentsMap as we did with removeFromDocumentsMap.

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:65
> +    static String toString(webm::TrackType type)

I think return value type should be ASCIILiteral, not String. Does that work?
Comment 9 Eric Carlson 2021-01-12 13:58:16 PST
Reopening to attach new patch.
Comment 10 Eric Carlson 2021-01-12 13:58:18 PST
Created attachment 417490 [details]
Address review comments
Comment 11 Eric Carlson 2021-01-12 14:02:20 PST
Comment on attachment 417216 [details]
Patch for landing

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

>> Source/WebCore/dom/Document.cpp:549
>> +    });
> 
> Is the emptiness check valuable? I’d expect allOf/values to efficiently do nothing for an empty map without an explicit check.

`allOf` return true for an empty map, and we don't want to allow logging if there are no documents.

>> Source/WebCore/dom/Document.cpp:643
>>      ASSERT_UNUSED(addResult, addResult.isNewEntry);
> 
> Seems like we should move this assertion into Document::addToDocumentsMap as we did with removeFromDocumentsMap.

Good point, moved.

>> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:65
>> +    static String toString(webm::TrackType type)
> 
> I think return value type should be ASCIILiteral, not String. Does that work?

It does, thanks for the suggestion.
Comment 12 Eric Carlson 2021-01-12 14:03:58 PST
(In reply to Darin Adler from comment #8)
> Why are we using "const void*" for identifiers instead of something that
> doesn't require reinterpret_cast. Is there some reason pretending this
> integer is a pointer has value?
> 
The log identifier is a void* because originally I thought that it would be more flexible, but in practice integers are used so we should change it.
Comment 13 EWS 2021-01-12 14:47:55 PST
Committed r271418: <https://trac.webkit.org/changeset/271418>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417490 [details].