Summary: | [Mac] Add runtime logging to format reader and WebM parser | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||
Component: | Media | Assignee: | 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
Eric Carlson
2021-01-07 11:12:39 PST
Created attachment 417208 [details]
Patch
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 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 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. Created attachment 417216 [details]
Patch for landing
Committed r271270: <https://trac.webkit.org/changeset/271270> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417216 [details]. 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? Reopening to attach new patch. Created attachment 417490 [details]
Address review comments
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. (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. Committed r271418: <https://trac.webkit.org/changeset/271418> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417490 [details]. |