Bug 223118 - Show media session image artwork in Now Playing information pane.
Summary: Show media session image artwork in Now Playing information pane.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on:
Blocks: 223290
  Show dependency treegraph
 
Reported: 2021-03-12 06:21 PST by Jean-Yves Avenard [:jya]
Modified: 2021-03-17 13:47 PDT (History)
12 users (show)

See Also:


Attachments
Patch (22.48 KB, patch)
2021-03-12 06:32 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (19.57 KB, patch)
2021-03-12 18:51 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (27.27 KB, patch)
2021-03-15 05:47 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (27.47 KB, patch)
2021-03-15 06:35 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (27.47 KB, patch)
2021-03-15 15:47 PDT, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (27.43 KB, patch)
2021-03-15 17:48 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (27.57 KB, patch)
2021-03-15 18:40 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (28.62 KB, patch)
2021-03-16 18:37 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jean-Yves Avenard [:jya] 2021-03-12 06:21:32 PST
Show media session image artwork in Now Playing information pane.
Comment 1 Jean-Yves Avenard [:jya] 2021-03-12 06:32:38 PST
Created attachment 423041 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-03-12 06:34:32 PST
<rdar://problem/75360166>
Comment 3 Eric Carlson 2021-03-12 08:58:54 PST
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 4 Jean-Yves Avenard [:jya] 2021-03-12 17:36:13 PST
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 .
Comment 5 Jean-Yves Avenard [:jya] 2021-03-12 18:51:22 PST
Created attachment 423093 [details]
Patch
Comment 6 Eric Carlson 2021-03-13 11:07:12 PST
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.
Comment 7 Jean-Yves Avenard [:jya] 2021-03-15 05:47:02 PDT
Created attachment 423161 [details]
Patch
Comment 8 Jean-Yves Avenard [:jya] 2021-03-15 06:24:41 PDT
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.
Comment 9 Jean-Yves Avenard [:jya] 2021-03-15 06:35:30 PDT
Created attachment 423171 [details]
Patch
Comment 10 Eric Carlson 2021-03-15 10:16:35 PDT
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.
Comment 11 Jean-Yves Avenard [:jya] 2021-03-15 15:47:44 PDT
Created attachment 423256 [details]
Patch
Comment 12 Chris Dumez 2021-03-15 15:56:11 PDT
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?
Comment 13 Jean-Yves Avenard [:jya] 2021-03-15 17:48:02 PDT
Created attachment 423274 [details]
Patch
Comment 14 Jean-Yves Avenard [:jya] 2021-03-15 18:24:08 PDT
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.
Comment 15 Jean-Yves Avenard [:jya] 2021-03-15 18:33:37 PDT
(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.
Comment 16 Jean-Yves Avenard [:jya] 2021-03-15 18:40:20 PDT
Created attachment 423278 [details]
Patch
Comment 17 Eric Carlson 2021-03-16 16:43:02 PDT
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.
Comment 18 Jean-Yves Avenard [:jya] 2021-03-16 18:37:06 PDT
Created attachment 423421 [details]
Patch
Comment 19 EWS 2021-03-17 13:46:56 PDT
Committed r274586: <https://commits.webkit.org/r274586>

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