Bug 223731 – Now Playing artwork doesn't update when changed.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jean-Yves Avenard [:jya]]

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.

[Radar WebKit Bug Importer]

<rdar://problem/75823923>

[Jean-Yves Avenard [:jya]]

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.

[Jean-Yves Avenard [:jya]]

Created attachment 424746 [details]
Patch

[youenn fablet]

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.

[Jean-Yves Avenard [:jya]]

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

[youenn fablet]

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

[Eric Carlson]

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?

[Jean-Yves Avenard [:jya]]

Created attachment 424848 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 424852 [details]
Patch

[Jean-Yves Avenard [:jya]]

not sure what webkit-patch did here, it uploaded the patch from another bug

[Jean-Yves Avenard [:jya]]

Created attachment 424863 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 424867 [details]
Patch

Add MediaUniqueIdentifier.h to cmake

[EWS]

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

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