Bug 223118 – Show media session image artwork in Now Playing information pane.

• Component: Media
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

Show media session image artwork in Now Playing information pane.

[Jean-Yves Avenard [:jya]]

Created attachment 423041 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75360166>

[Eric Carlson]

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/

[Jean-Yves Avenard [:jya]]

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 .

[Jean-Yves Avenard [:jya]]

Created attachment 423093 [details]
Patch

[Eric Carlson]

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.

[Jean-Yves Avenard [:jya]]

Created attachment 423161 [details]
Patch

[Jean-Yves Avenard [:jya]]

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.

[Jean-Yves Avenard [:jya]]

Created attachment 423171 [details]
Patch

[Eric Carlson]

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.

[Jean-Yves Avenard [:jya]]

Created attachment 423256 [details]
Patch

[Chris Dumez]

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?

[Jean-Yves Avenard [:jya]]

Created attachment 423274 [details]
Patch

[Jean-Yves Avenard [:jya]]

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.

[Jean-Yves Avenard [:jya]]

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

[Jean-Yves Avenard [:jya]]

Created attachment 423278 [details]
Patch

[Eric Carlson]

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.

[Jean-Yves Avenard [:jya]]

Created attachment 423421 [details]
Patch

[EWS]

Committed r274586: <https://commits.webkit.org/r274586>

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