Show media session image artwork in Now Playing information pane.
Created attachment 423041 [details] Patch
<rdar://problem/75360166>
Comment on attachment 423041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423041&action=review > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:49 > + ArtworkImageLoader(Document& document, String src, ArtworkImageLoaderCallback&& callback) const String& src > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:60 > + m_cachedImage = nullptr; I don't think the is necessary > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:80 > + void notifyFinished(CachedResource&, const NetworkLoadMetrics&) override > + { > + ASSERT(m_cachedImage); Maybe `ASSERT_UNUSED(resource, &resource == m_cachedImage)` ? > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:82 > + bool loadSucceeded = !m_cachedImage->errorOccurred() && m_cachedImage->response().httpStatusCode() < 400; > + m_callback(loadSucceeded ? m_cachedImage->image() : nullptr); Why not just use something like `m_callback(m_cachedImage->loadFailedOrCanceled() ? m_cachedImage->image() : nullptr)`, does that not check response status? > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:112 > + // We can now start retrieving the artwork image if it hadn't been done before. I'm not sure this comment helps, the method name makes it clear what happens. > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:172 > + if (m_session && m_session->document() && mediaImages[0].src != m_artworkImageSrc) { 1. Won't `mediaImages[0].src` crash if m_metadata.artwork is empty? 2. You should add a `FIXME:` here about coming up with a heuristic to choose the "best" image. 3. If the images array is now empty and there is load in progress, you should cancel it 4. WebKit style is to return early instead of indenting the rest of the function > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:174 > + m_artworkLoader = WTF::makeUnique<ArtworkImageLoader>(*m_session->document(), m_artworkImageSrc, [weakThis = makeWeakPtr(*this), this, artworkId = ++m_artworkId](Image* image) { I don't think the "WTF::" is necessary if you #include <wtf/StdLibExtras.h> > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:175 > + if (weakThis && artworkId == m_artworkId) { Wouldn't it be more efficient, and cleaner, to clear m_artworkLoader if it exists cancel the pending load before starting a new one instead of using this ID check? > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:178 > + WTFLogAlways("Received artwork image:%s mimeType:%s artworkId:%u", m_artworkImageSrc.characters<LChar>(), > + image->mimeType().characters<LChar>(), artworkId); I assume this is just temporary debugging code, but it would be useful to log at runtime so let play to have the MediaSession set a Logger in `MediaSession::setMetadata` > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:199 > +Optional<IntSize> MediaMetadata::maybeSize(String sizes) const This method is unused? > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:204 > + // the attribute must have a value that is an unordered set of unique space-separated tokens which are ASCII case-insensitive. > + // Each value must be either an ASCII case-insensitive match for the string "any", or a value that consists of two valid non-negative > + // integers that do not have a leading U+0030 DIGIT ZERO (0) character and that are separated by a single U+0078 LATIN SMALL LETTER X > + // or U+0058 LATIN CAPITAL LETTER X character. If this is spec text, we typically include a link to the source > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:207 > + size_t length = tokens.size(); > + for (size_t i = 0; i < length; ++i) { This can be made more compact (without loss of clarity IMO): for (size_t i = 0, length = tokens.size(); i < length; ++i) { > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:209 > + if (tokens[i] == "any") > + return { }; Can you check for "any" once, before the loop, instead of checking each token? > Source/WebCore/Modules/mediasession/MediaSession.cpp:183 > + if (!m_navigator) > + return nullptr; > + return m_navigator->window()->document(); `m_navigator->window()` can be NULL > Source/WebCore/platform/audio/NowPlayingInfo.h:52 > + || (imageData && other.imageData && imageData.get() != imageData.get() && *imageData == *imageData); I don't understand `imageData.get() != imageData.get() && *imageData == *imageData` > Source/WebCore/platform/audio/NowPlayingInfo.h:86 > + double duration { 0 }; Nit: an extra space snuck in here > Source/WebCore/platform/audio/NowPlayingInfo.h:109 > + bool operator!=(const NowPlayingInfo& other) const Oops! > Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:378 > + // TODO: artwork isn't refreshed typically. There must be a better way to do this. s/TODO/FIXME/
Comment on attachment 423041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423041&action=review >> Source/WebCore/Modules/mediasession/MediaMetadata.cpp:60 >> + m_cachedImage = nullptr; > > I don't think the is necessary nulling the member is what a few classes implementing CachedResourceClient are doing. I thought this was a style thing. >> Source/WebCore/Modules/mediasession/MediaMetadata.cpp:199 >> +Optional<IntSize> MediaMetadata::maybeSize(String sizes) const > > This method is unused? it's used by the code that determine the "best" image, which I didn't upload. >> Source/WebCore/Modules/mediasession/MediaMetadata.cpp:204 >> + // or U+0058 LATIN CAPITAL LETTER X character. > > If this is spec text, we typically include a link to the source there's no direct link to that verbiage unfortunately. >> Source/WebCore/Modules/mediasession/MediaMetadata.cpp:209 >> + return { }; > > Can you check for "any" once, before the loop, instead of checking each token? doing so after the split allows to easily deal with any spaces .
Created attachment 423093 [details] Patch
Comment on attachment 423093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423093&action=review This looks good, but I think it is important to add a test before landing. > Source/WebCore/ChangeLog:9 > + Manually tested, no framework available to test this feature. We can at least test image loader. You could, for example, add a method to Internals that returns a promise which resolves with the artwork image once it loads. See Internals::grabNextMediaStreamTrackFrame and the test LayoutTests/fast/mediastream/captureStream/canvas3d.html for one way this could be done.
Created attachment 423161 [details] Patch
Turns out, using a CompletionHandler isn't suitable as it must run at least once. We don't want to run it in the ArtworkImageLoader destructor as we don't know where we're up to in the destructor of the artwork loader owner.
Created attachment 423171 [details] Patch
Comment on attachment 423171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423171&action=review > Source/WebCore/testing/Internals.cpp:6109 > +void Internals::loadArtworkImage(String&& url, ArtworkImagePromise&& promise) > +{ > + if (!contextDocument()) { > + promise.reject(Exception { InvalidStateError, "No document." }); > + return; > + } > + m_artworkImagePromise = WTF::makeUnique<ArtworkImagePromise>(WTFMove(promise)); > + m_artworkLoader = makeUnique<ArtworkImageLoader>(*contextDocument(), url, [this](Image* image) { > + if (image) { > + auto imageData = ImageData::create(unsigned(image->width()), unsigned(image->height())); > + if (!imageData.hasException()) > + m_artworkImagePromise->resolve(imageData.releaseReturnValue()); > + else > + m_nextTrackFramePromise->reject(imageData.exception().code()); > + } else > + m_artworkImagePromise->reject(Exception { InvalidAccessError, "No image retrieve." }); > + m_artworkImagePromise = nullptr; > + }); > + m_artworkLoader->requestImageResource(); > +} This is fine for now, but lets change this once https://bugs.webkit.org/show_bug.cgi?id=222158 lands to have Internals register as an artwork observer and have the test kick off an implicit load by setting MediaSession.metadata. > LayoutTests/fast/mediasession/metadata/artworkdownload-expected.txt:2 > +FAIL ensure loading artwork image method operates properly assert_equals: expected 18 but got 16 it looks like the test and results don't match.
Created attachment 423256 [details] Patch
Comment on attachment 423256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423256&action=review > Source/WebCore/Modules/mediasession/MediaMetadata.h:51 > + typedef WTF::Function<void(Image*)> ArtworkImageLoaderCallback; We prefer 'using' statements nowadays: using ArtworkImageLoaderCallback = Function<void(Image*)>; Also note that you don't need WTF:: > Source/WebCore/testing/Internals.cpp:5260 > + m_nextTrackFramePromise = WTF::makeUnique<TrackFramePromise>(WTFMove(promise)); WTF:: is not needed. > Source/WebCore/testing/Internals.cpp:6096 > + m_artworkImagePromise = WTF::makeUnique<ArtworkImagePromise>(WTFMove(promise)); ditto. > Source/WebCore/testing/Internals.cpp:6097 > + m_artworkLoader = makeUnique<ArtworkImageLoader>(*contextDocument(), url, [this](Image* image) { Shouldn't we handle the case where m_artworkLoader is non-null?
Created attachment 423274 [details] Patch
Comment on attachment 423256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423256&action=review >> Source/WebCore/testing/Internals.cpp:5260 >> + m_nextTrackFramePromise = WTF::makeUnique<TrackFramePromise>(WTFMove(promise)); > > WTF:: is not needed. This is existing code though, and there's plenty of places using WTF:: in this file >> Source/WebCore/testing/Internals.cpp:6097 >> + m_artworkLoader = makeUnique<ArtworkImageLoader>(*contextDocument(), url, [this](Image* image) { > > Shouldn't we handle the case where m_artworkLoader is non-null? makeUnique (and new) is infallible AFAIK.
(In reply to Chris Dumez from comment #12) > > Source/WebCore/testing/Internals.cpp:6097 > > + m_artworkLoader = makeUnique<ArtworkImageLoader>(*contextDocument(), url, [this](Image* image) { > > Shouldn't we handle the case where m_artworkLoader is non-null? I seem to have misread your comment. I think you're right, we should handle the case where we have a pending download and reject the previous one.
Created attachment 423278 [details] Patch
Comment on attachment 423278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423278&action=review > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:57 > + ResourceLoaderOptions options = CachedResourceLoader::defaultCachedResourceOptions(); Although it isn't an issue with the current code, this method *could* be called while an existing load is in progress so if m_cachedImage is non-null you should probably remove the client and free it here. Maybe add a `clearCachedImage` method and use it here and from the destructor? > Source/WebCore/html/MediaElementSession.cpp:1116 > + artwork = NowPlayingInfoArtwork { sessionMetadata->artworkSrc(), sessionMetadata->artworkImage()->mimeType(), sessionMetadata->artworkImage()->data() }; Please add a FIXME here about the optimization of only sending image data when it changes.
Created attachment 423421 [details] Patch
Committed r274586: <https://commits.webkit.org/r274586> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423421 [details].