Bug 220423 – [Mac] Add runtime logging to format reader and WebM parser

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

[Eric Carlson]

Add runtime logging to format reader and WebM parser to make failures easier to diagnose

[Radar WebKit Bug Importer]

<rdar://problem/72896655>

[Eric Carlson]

Created attachment 417208 [details]
Patch

[Andy Estes]

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?

[Andy Estes]

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()`.

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 417216 [details]
Patch for landing

[EWS]

Committed r271270: <https://trac.webkit.org/changeset/271270>

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

[Darin Adler]

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?

[Eric Carlson]

Reopening to attach new patch.

[Eric Carlson]

Created attachment 417490 [details]
Address review comments

[Eric Carlson]

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.

[Eric Carlson]

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

[EWS]

Committed r271418: <https://trac.webkit.org/changeset/271418>

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