WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
223118
Show media session image artwork in Now Playing information pane.
https://bugs.webkit.org/show_bug.cgi?id=223118
Summary
Show media session image artwork in Now Playing information pane.
Jean-Yves Avenard [:jya]
Reported
2021-03-12 06:21:32 PST
Show media session image artwork in Now Playing information pane.
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
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Jean-Yves Avenard [:jya]
Comment 1
2021-03-12 06:32:38 PST
Created
attachment 423041
[details]
Patch
Radar WebKit Bug Importer
Comment 2
2021-03-12 06:34:32 PST
<
rdar://problem/75360166
>
Eric Carlson
Comment 3
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/
Jean-Yves Avenard [:jya]
Comment 4
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 .
Jean-Yves Avenard [:jya]
Comment 5
2021-03-12 18:51:22 PST
Created
attachment 423093
[details]
Patch
Eric Carlson
Comment 6
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.
Jean-Yves Avenard [:jya]
Comment 7
2021-03-15 05:47:02 PDT
Created
attachment 423161
[details]
Patch
Jean-Yves Avenard [:jya]
Comment 8
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.
Jean-Yves Avenard [:jya]
Comment 9
2021-03-15 06:35:30 PDT
Created
attachment 423171
[details]
Patch
Eric Carlson
Comment 10
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.
Jean-Yves Avenard [:jya]
Comment 11
2021-03-15 15:47:44 PDT
Created
attachment 423256
[details]
Patch
Chris Dumez
Comment 12
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?
Jean-Yves Avenard [:jya]
Comment 13
2021-03-15 17:48:02 PDT
Created
attachment 423274
[details]
Patch
Jean-Yves Avenard [:jya]
Comment 14
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.
Jean-Yves Avenard [:jya]
Comment 15
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.
Jean-Yves Avenard [:jya]
Comment 16
2021-03-15 18:40:20 PDT
Created
attachment 423278
[details]
Patch
Eric Carlson
Comment 17
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.
Jean-Yves Avenard [:jya]
Comment 18
2021-03-16 18:37:06 PDT
Created
attachment 423421
[details]
Patch
EWS
Comment 19
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug