Bug 223731 - Now Playing artwork doesn't update when changed.
Summary: Now Playing artwork doesn't update when changed.
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: 223795
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-24 23:20 PDT by Jean-Yves Avenard [:jya]
Modified: 2021-04-01 08:17 PDT (History)
15 users (show)

See Also:


Attachments
Patch (18.13 KB, patch)
2021-03-31 02:17 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (24.18 KB, patch)
2021-03-31 17:44 PDT, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2021-03-31 18:04 PDT, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (24.22 KB, patch)
2021-03-31 19:03 PDT, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (24.75 KB, patch)
2021-03-31 19:41 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-24 23:20:45 PDT
In bug 223118 we added support for artwork image.

When a new artwork is found, we call MediaRemote with the following keys:
        CFDictionarySetValue(info.get(), kMRMediaRemoteNowPlayingInfoArtworkData, nsArtwork.get());
        CFDictionarySetValue(info.get(), kMRMediaRemoteNowPlayingInfoArtworkMIMEType, nowPlayingInfo.artwork->mimeType.createCFString().get());
        CFDictionarySetValue(info.get(), kMRMediaRemoteNowPlayingInfoArtworkIdentifier, nowPlayingInfo.artwork->src.createCFString().get());

The artwork only gets refreshed once when the user click in macOS Now Playing menu bar and click on pause/play. It doesn't get updated from then on.

This bug is hoping to find the reason on why that would be the case.
Comment 1 Radar WebKit Bug Importer 2021-03-24 23:20:53 PDT
<rdar://problem/75823923>
Comment 2 Jean-Yves Avenard [:jya] 2021-03-30 22:52:05 PDT
bug 208710 and bug 210518 combined the HTMLMediaElement unique identified and MediaSession unique identifier and this is what is being sent to MediaRemote along the Now Playing information.

The MediaSession unique identifier is as its name suggests, unique. Even if the media element is playing a different title, the content identifier that will be sent along the Now Playing CFDictionary will remain constant for the life of the web content process.

This causes information to not be refreshed as you would expect.

Additionally, two separate bugs in the Control Manager cause the artwork to not be refreshed when a new one is set.

1- The Control Manager apparently doesn't listen to change in the kMRMediaRemoteNowPlayingInfoArtworkIdentifier so if new artwork is sent after kMRMediaRemoteNowPlayingInfoUniqueIdentifier has been sent, we will continue to see the old image.

2- After kMRMediaRemoteNowPlayingInfoUniqueIdentifier changing, if we go from a Now Playing session with artwork to one without artwork, the old artwork will continue to show.

bug #1 is exposed due to how we asynchronously retrieve the artwork. We set a new Now Playing information dictionary with the title/artist/album (as those are retrieved synchronously) and call MediaRemote with those. Only once the image has been retrieved doe we call again MediaRemote with the artwork. 

In the incoming patch we will get around #1, for #2 a radar has been opened.
Comment 3 Jean-Yves Avenard [:jya] 2021-03-31 02:17:45 PDT
Created attachment 424746 [details]
Patch
Comment 4 youenn fablet 2021-03-31 03:38:34 PDT
Comment on attachment 424746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424746&action=review

> Source/WebCore/ChangeLog:11
> +        <rdar://problem/75823923>

We usually try to have these two lines just after the title and before the comments.

> Source/WebCore/html/HTMLMediaElement.cpp:7515
> +uint64_t HTMLMediaElement::mediaUniqueIdentifier() const

This seems like a regression to untype the id.
I would create a MediaUniqueIdentifier if we cannot use MediaSessionIdentifier.

That said, maybe we should just pass m_currentSrc.string() as part of the NowPlayingInfo.
It will then be up to the platform specific code to generate a unique ID from it, if it needs to.
Comment 5 Jean-Yves Avenard [:jya] 2021-03-31 04:03:23 PDT
(In reply to youenn fablet from comment #4)

> > Source/WebCore/html/HTMLMediaElement.cpp:7515
> > +uint64_t HTMLMediaElement::mediaUniqueIdentifier() const
> 
> This seems like a regression to untype the id.
> I would create a MediaUniqueIdentifier if we cannot use
> MediaSessionIdentifier.

the id is only ever used as an uint64, and is the type to set in the CFDictionary passed to MediaRemote.

So passing a MediaUniqueIdentifier which ends up being just a uint64_t seems unwarranted.

> 
> That said, maybe we should just pass m_currentSrc.string() as part of the
> NowPlayingInfo.
> It will then be up to the platform specific code to generate a unique ID
> from it, if it needs to.

I thought about that, but I wasn't 100% certain giving the actual src to something outside webkit was okay, such as privacy concern.
Comment 6 youenn fablet 2021-03-31 04:16:25 PDT
(In reply to Jean-Yves Avenard [:jya] from comment #5)
> (In reply to youenn fablet from comment #4)
> 
> > > Source/WebCore/html/HTMLMediaElement.cpp:7515
> > > +uint64_t HTMLMediaElement::mediaUniqueIdentifier() const
> > 
> > This seems like a regression to untype the id.
> > I would create a MediaUniqueIdentifier if we cannot use
> > MediaSessionIdentifier.
> 
> the id is only ever used as an uint64, and is the type to set in the
> CFDictionary passed to MediaRemote.
> 
> So passing a MediaUniqueIdentifier which ends up being just a uint64_t seems
> unwarranted.

Given the mediaUniqueIdentifier name, it seems good to understand the scope of its uniqueness.
uint64_t does not give that, MediaUniqueIdentifier will be easier to reason about.

The thing I am not too clear is whether we want uniqueness only in WebProcess or if it is important that uniqueness also happens for all pages when setting playing info in GPUProcess.
In the latter case, ObjectIdentifier will not give that, we would need ProcessID+ObjectIdentifier.

> > That said, maybe we should just pass m_currentSrc.string() as part of the
> > NowPlayingInfo.
> > It will then be up to the platform specific code to generate a unique ID
> > from it, if it needs to.
> 
> I thought about that, but I wasn't 100% certain giving the actual src to
> something outside webkit was okay, such as privacy concern.

Usually, strings like that would not by default be logged but I see your point.
How about passing currentUrlSrc, which is easy to understand, and set kMRMediaRemoteNowPlayingInfoArtworkIdentifier to currentUrlSrc.hash() down in Cocoa specific code?
Comment 7 Eric Carlson 2021-03-31 12:54:23 PDT
Comment on attachment 424746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424746&action=review

> Source/WebCore/ChangeLog:8
> +        If the same identifier is provided for every call to MRMediaRemoteSetNowPlayingInfo, then some updates may be skipped.
> +        In earlier changes, the MediaSessionElement identifier was combined with the Media Element identifier which broke Now Playing as the MediaSession identifier never changes for the lifetime of the web content process.
> +        So we create a new method HTMLMediaElement::mediaUniqueIdentifier that will use a hash based on the src string attribute
> +        and once again separate the MediaSession identifier from the Media Element identifier.

Nit: please be kind to those of us that use smaller screen and wrap ChangeLog lines to a reasonable width (e.g. 100)

> Source/WebCore/ChangeLog:18
> +        (WebCore::HTMLMediaElement::mediaUniqueIdentifier const): Rename method to more clearly reflect the difference between a MediaSession identifier and the actual content being played.

Ditto.

> Source/WebCore/ChangeLog:21
> +        (WebCore::MediaElementSession::nowPlayingInfo const): use renamed method, so that we can uniquely identify the content being played rather than which MediaSessionElement is being used.

Ditto.

> Source/WebCore/ChangeLog:28
> +        (WebCore::MediaSessionManagerCocoa::setNowPlayingInfo): set kMRMediaRemoteNowPlayingInfoArtworkIdentifier in CFDictionary to the source of the artwork. Workaround a bug in Media Controller component.

Ditto.

> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:310
> +        CFDictionarySetValue(info.get(), kMRMediaRemoteNowPlayingInfoArtworkIdentifier, nowPlayingInfo.artwork->src.createCFString().get());

I agree that it would be better to not pass the URL because WebKit has a strict policy of never logging URLs, and we can't guarantee that NowPlaying has the same policy. 

Why not use the same identifier you use for kMRMediaRemoteNowPlayingInfoUniqueIdentifier?
Comment 8 Jean-Yves Avenard [:jya] 2021-03-31 17:44:27 PDT
Created attachment 424848 [details]
Patch
Comment 9 Jean-Yves Avenard [:jya] 2021-03-31 18:04:51 PDT
Created attachment 424852 [details]
Patch
Comment 10 Jean-Yves Avenard [:jya] 2021-03-31 18:18:00 PDT
not sure what webkit-patch did here, it uploaded the patch from another bug
Comment 11 Jean-Yves Avenard [:jya] 2021-03-31 19:03:07 PDT
Created attachment 424863 [details]
Patch
Comment 12 Jean-Yves Avenard [:jya] 2021-03-31 19:41:39 PDT
Created attachment 424867 [details]
Patch

Add MediaUniqueIdentifier.h to cmake
Comment 13 EWS 2021-04-01 08:17:32 PDT
Committed r275359: <https://commits.webkit.org/r275359>

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