MediaSession artwork only uses the first item in the artwork array by jyavenard · Pull Request #30598 · WebKit/WebKit

jyavenard · 2024-07-09T08:20:06Z

`cbbcae6`

MediaSession artwork only uses the first item in the artwork array
https://bugs.webkit.org/show_bug.cgi?id=276351
rdar://81160539

Reviewed by Youenn Fablet.

If `sizes` metadata artwork attribute is provided we will use them to determine the "best"
image to select. We do so by giving each resolution a score between 0 and 1. We aim to download
an image that is 512x512 with an aspect ratio of 1.
If the `sizes` attribute contains invalid content or if not provided, we will iterate through
all the available images and download them. We will then either select the one with the highest
score, or the first one found with a resolution greater than 512 pixels.

Test will be added once https://bugs.webkit.org/show_bug.cgi?id=276133 lands as it adds some
internal APIs to check which URL was selected with Now Playing.
For now, manually tested.

* Source/WebCore/Modules/mediasession/MediaMetadata.cpp:
(WebCore::MediaMetadata::imageDimensionsScore const):
(WebCore::MediaMetadata::refreshArtworkImage):
(WebCore::MediaMetadata::tryNextArtworkImage):
* Source/WebCore/Modules/mediasession/MediaMetadata.h:

Canonical link: https://commits.webkit.org/280804@main

515c507

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2
	~~🛠 tv~~
	✅ 🛠 tv-sim
	~~🛠 watch~~
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-07-09T08:20:33Z

EWS run on previous version of this PR (hash 51d7516)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2
	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

youennf · 2024-07-09T09:01:42Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

Suggested change

for (auto element : stringView.split(' ')) {

for (auto element : StringView(sizes).split(' ')) {

Also do we handle the case of multiple spaces like 192x192 320x320?

Also do we handle the case of multiple spaces like 192x192 320x320?

Hmm I didn't think of that. If spaces are correct then we take the biggest of the series

youennf · 2024-07-09T09:03:29Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

Suggested change

Vector<std::optional<uint32_t>> numbers;

Vector<std::optional<uint32_t>, 2> numbers;

removed that code altogether

youennf · 2024-07-09T09:07:34Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

We can probably use findIgnoringASCIICase("x"_s) to remove this string allocation.

youennf · 2024-07-09T09:13:16Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

I prefer the old version of checking whether image is null upfront.
The current version is ok I think for that case, but somehow convoluted.

I prefer the old version of checking whether image is null upfront. The current version is ok I think for that case, but somehow convoluted.

we don't want to exit early when it's null anymore, we want to try the other artwork

Sure, wouldn't the following just work:

if (!image) { tryNextArtworkImage(index + 1); return; } m_artworkImageSrc = artworkImageSrc; setArtworkImage(image); metadataUpdated(); // Maybe we should also free m_artworkLoader, need to check how this loader is implemented.

youennf · 2024-07-09T09:14:29Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

s/iterate/shouldIterate/

removed that code

youennf · 2024-07-09T09:22:10Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

Suggested change

if (!iterate || index + 1 < m_metadata.artwork.size())

if (!iterate || ++index < m_metadata.artwork.size())

this would require to make the entire lambda mutable

youennf · 2024-07-09T09:23:20Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

Suggested change

IntSize newSize = { int(*numbers[0]), int(*numbers[1]) };

IntSize newSize { int(*numbers[0]), int(*numbers[1]) };

youennf · 2024-07-09T09:29:24Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

Would it be simpler to have a Vector of image srcs, sorted by the image score.
Then, start with the first image src (the one with the best score) and only iterate if there is no image?

If we can sort the src, then we only retrieve one image. There's no need to keep sorted vector anymore.

Otherwise we can't sort until we have downloaded all the images.

yeah, it ended up being nicer that way. and it simplifies the logic as we don't need to distinguish the case where we do need to iterate for those we don't

webkit-early-warning-system · 2024-07-09T11:34:59Z

EWS run on previous version of this PR (hash b068ce2)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2
	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

youennf · 2024-07-09T12:30:10Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

Theoretically, it would be possible for m_idealSize and m_minimumSize to be the same.
Given they are static, we should probably rename them as s_...
Or maybe make this method a free function and have const variables for ideal/minimum sizes.

I added a static_assert

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

youennf · 2024-07-09T12:33:39Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

Suggested change

m_sortedSrc.clear();

m_sortedSrces.clear();

youennf · 2024-07-09T12:34:45Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

Sure, wouldn't the following just work:

if (!image) { tryNextArtworkImage(index + 1); return; } m_artworkImageSrc = artworkImageSrc; setArtworkImage(image); metadataUpdated(); // Maybe we should also free m_artworkLoader, need to check how this loader is implemented.

youennf · 2024-07-09T12:35:15Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

Suggested change

float imageScore = size.isEmpty() ? -1.0 : imageDimensionsScore(size.width(), size.height());

float imageScore = imageDimensionsScore(size.width(), size.height());

youennf · 2024-07-09T12:36:51Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

Suggested change

tryNextArtworkImage(index + 1);

tryNextArtworkImage(index);

youennf · 2024-07-09T12:40:09Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

I would tend to do the following:

Suggested change

void MediaMetadata::tryNextArtworkImage(uint32_t index)

void MediaMetadata::tryNextArtworkImage(Vector<Pair>&&, uint32_t index = 0)

And have the lambda capture the Vector.
Also m_sortedSrcand m_artworkLoader are useless once we finished loading. If it was captured in the lambda, it would be freed automatically by freeing m_artworkLoader

youennf · 2024-07-09T12:40:45Z

Source/WebCore/Modules/mediasession/MediaMetadata.h

Suggested change

static constexpr int m_minimumSize = 128;

static constexpr int s_minimumSize = 128;

youennf · 2024-07-09T12:40:54Z

Source/WebCore/Modules/mediasession/MediaMetadata.h

Suggested change

static constexpr int m_idealSize = 512;

static constexpr int s_idealSize = 512;

youennf · 2024-07-09T12:41:32Z

Source/WebCore/Modules/mediasession/MediaMetadata.cpp

Would it be possible to directly create m_sortedSrc without creating this sizes vector?

youennf · 2024-07-09T12:47:54Z

Test will be added once https://bugs.webkit.org/show_bug.cgi?id=276133 lands as it adds some
internal APIs to check which URL was selected with Now Playing.

Let's not forget to add some edge cases like 192x X56, multiple spaces...

youennf

LGTM.
Some further improvements added in comments.

webkit-early-warning-system · 2024-07-10T00:52:53Z

EWS run on current version of this PR (hash 515c507)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2
	~~🛠 tv~~
	✅ 🛠 tv-sim
	~~🛠 watch~~
	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=276351 rdar://81160539 Reviewed by Youenn Fablet. If `sizes` metadata artwork attribute is provided we will use them to determine the "best" image to select. We do so by giving each resolution a score between 0 and 1. We aim to download an image that is 512x512 with an aspect ratio of 1. If the `sizes` attribute contains invalid content or if not provided, we will iterate through all the available images and download them. We will then either select the one with the highest score, or the first one found with a resolution greater than 512 pixels. Test will be added once https://bugs.webkit.org/show_bug.cgi?id=276133 lands as it adds some internal APIs to check which URL was selected with Now Playing. For now, manually tested. * Source/WebCore/Modules/mediasession/MediaMetadata.cpp: (WebCore::MediaMetadata::imageDimensionsScore const): (WebCore::MediaMetadata::refreshArtworkImage): (WebCore::MediaMetadata::tryNextArtworkImage): * Source/WebCore/Modules/mediasession/MediaMetadata.h: Canonical link: https://commits.webkit.org/280804@main

webkit-commit-queue · 2024-07-10T03:27:13Z

Committed 280804@main (cbbcae6): https://commits.webkit.org/280804@main

Reviewed commits have been landed. Closing PR #30598 and removing active labels.

jyavenard self-assigned this Jul 9, 2024

jyavenard added the Media Bugs related to the HTML 5 Media elements. label Jul 9, 2024

jyavenard requested a review from a team July 9, 2024 08:24

youennf reviewed Jul 9, 2024

View reviewed changes

jyavenard force-pushed the eng/MediaSession-artwork-only-uses-the-first-item-in-the-artwork-array branch from 51d7516 to b068ce2 Compare July 9, 2024 11:34

jyavenard requested a review from youennf July 9, 2024 11:37

youennf reviewed Jul 9, 2024

View reviewed changes

youennf approved these changes Jul 9, 2024

View reviewed changes

jyavenard force-pushed the eng/MediaSession-artwork-only-uses-the-first-item-in-the-artwork-array branch from b068ce2 to 515c507 Compare July 10, 2024 00:52

jyavenard added the merge-queue Applied to send a pull request to merge-queue label Jul 10, 2024

webkit-commit-queue force-pushed the eng/MediaSession-artwork-only-uses-the-first-item-in-the-artwork-array branch from 515c507 to bbeb5ef Compare July 10, 2024 03:25

webkit-commit-queue force-pushed the eng/MediaSession-artwork-only-uses-the-first-item-in-the-artwork-array branch from bbeb5ef to cbbcae6 Compare July 10, 2024 03:27

webkit-commit-queue merged commit cbbcae6 into WebKit:main Jul 10, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 10, 2024

jyavenard deleted the eng/MediaSession-artwork-only-uses-the-first-item-in-the-artwork-array branch November 27, 2024 23:50

	for (auto element : stringView.split(' ')) {
	for (auto element : StringView(sizes).split(' ')) {

	Vector<std::optional<uint32_t>> numbers;
	Vector<std::optional<uint32_t>, 2> numbers;

	if (!iterate \|\| index + 1 < m_metadata.artwork.size())
	if (!iterate \|\| ++index < m_metadata.artwork.size())

	IntSize newSize = { int(numbers[0]), int(numbers[1]) };
	IntSize newSize { int(numbers[0]), int(numbers[1]) };

	float imageScore = size.isEmpty() ? -1.0 : imageDimensionsScore(size.width(), size.height());
	float imageScore = imageDimensionsScore(size.width(), size.height());

	void MediaMetadata::tryNextArtworkImage(uint32_t index)
	void MediaMetadata::tryNextArtworkImage(Vector<Pair>&&, uint32_t index = 0)

	static constexpr int m_minimumSize = 128;
	static constexpr int s_minimumSize = 128;

	static constexpr int m_idealSize = 512;
	static constexpr int s_idealSize = 512;

Conversation

jyavenard commented Jul 9, 2024 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youennf commented Jul 9, 2024

Uh oh!

youennf left a comment

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented Jul 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jyavenard commented Jul 9, 2024 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Jul 9, 2024 •

edited

Loading

webkit-early-warning-system commented Jul 9, 2024 •

edited

Loading

webkit-early-warning-system commented Jul 10, 2024 •

edited

Loading