1623971 - [MediaControl-Windows] Show artwork image on the virtual control interface (SMTC)

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, task, P1)

Description

[Alastor Wu [:alwu]]

In bug1620340, we have shown the artist, album and title on the virtual control interface. However, we haven't set the artwork image yet.

This bug is going to implement the last piece of showing complete metadata on the virtual control interface.

Comment 1

[Alastor Wu [:alwu]]

Hi, Marc,
I guess that you might want to implement this? But if you don't have time to do so, free feel to re-assign this bug to me.
Thank you.

Comment 2

[Marc Streckfuß [:MeFisto94]]

Attachment: Bug 1623971 - part1: Propagate the MediaSession Artwork Images to MPRIS, r=alwu

Comment 3

[Tom S [:evilpie]]

I am not comfortable with the current approach. I assume the artwork URL is settable by webcontent? This would expose a huge new attack surface to the web. At least the HTTP stack and image decoding. I doubt most desktop environments even have sandboxing for this.

I think it would be better if we created the artwork image ourself in a temp file and passed a file: URL to MPRIS,

Comment 4

[Marc Streckfuß [:MeFisto94]]

That only mitigates the HTTP Stack in that case as the image would be the same, unless we'd be re-encoding the image in some way.

Comment 5

[Marc Streckfuß [:MeFisto94]]

Attachment: Bug 1623971 - part2: Propagate the MediaSession Artwork Images to Windows SystemMediaTransportControl. r=alwu,thomasmo

Comment 6

[Alastor Wu [:alwu]]

Comment 3 show some concern about letting media frameworks (MPRIS/SMTC) to fetch image directly, rather than fetching image on Firefox. But I don't have an idea about if that is something we definitely have to prevent.

Here are some short brief-up.
Websites can set the artwork image by providing an image URL [1], and we would show that image on the virtual control interface that platform provides. If websites didn't set the artwork image, then we can use our default image (maybe website's favicon, or our branding logo) instead.

Problem1
Should we fetch the image inside Firefox or let the platform media frameworks do that instead?

On Linux, MPRIS only accepts image URL that can be web-based protocal or file-based protocal [2], so if we want to show the image on Linux, we have to either passing URL to MPRIS directly or fecthing image on Firefox and storing it as a local file then pass that file-based URL to MPRIS (sounds not pratical). Chromium doesn't implement artwork image for MPRIS [3].

On Windows, SMTC provides a way to access the image from a bitmap [3] which is a way Chromium uses.

I wonder if there is any security concern about exposing image URL to media framework directly, because I don't have enough knowledge to evaluate that.

Problem2
When website doesn't provide the artwork image, we can use either website's favicon or our branding image. Considering the consistency among different websites and providing user a better hint about what application is playing music, we would like to use the branding image as artwork image because we have already had them on local disk.

The question is that, if there any way to get a robust file-based URL for the branding image? because the way used in [4] seems not really robust, if user changes firefox's local location, then we can't access the branding image. Or do we have a web-hosted server to perserve those image, then we can fetch them and pass the image to media framework? But it also introduces a new potential problem that is probably causing a DDOS to our server if too many users are accessing the same image at the same time.

[1] https://w3c.github.io/mediasession/#dom-mediaimage-src
[2] https://www.freedesktop.org/wiki/Specifications/mpris-spec/metadata/#uri
[3] Chromium implementation
Linux
https://cs.chromium.org/chromium/src/components/system_media_controls/linux/system_media_controls_linux.cc?type=cs&sq=package:chromium&g=0&l=126-140
Windows
https://cs.chromium.org/chromium/src/components/system_media_controls/win/system_media_controls_win.cc?type=cs&sq=package:chromium&g=0&l=217-219
[4] https://phabricator.services.mozilla.com/D67745

---

Hi, Baku, Tim,
I would like to hear your guys' suggestion about the problem1, which is about the security.
And also, if you know the answer of the problem2, or know who we can ask, please free feel to transfer NI to them.
Thank you so much.

Comment 7

[Andrea Marchesini [:baku]]

Problem1
Should we fetch the image inside Firefox or let the platform media frameworks do that instead?

There are several considerations to do here:

1. Do we need to send cookies? If the URL is loaded by external sources, they do not have access to the cookies.
2. Do we need to cache this content?
3. If the resource is fetched by an external component, our anti-tracking/dfpi/fpi features are broken for that URL.
   So yes, I think we should do the fetching internally. The way it should be done must be described in the spec. If not, I would suggest to do not share cookies, and to do not use the cache.

About the second problem, you can obtain the branding icon as a resource: URL. If you need to convert it in a file, you can copy into in a temporary folder.

Comment 8

[Tim Huang[:timhuang]]

I think the platform media framework itself would become an attack surface if we load resources through it.

1. For privacy, the media framework would know which website the user visits by inspecting the image URL. This is not desirable given the fact that we are trying to restrict the information that can be exposed between processes in Fission work.
2. For security, this allows the site to load arbitrary URLs in the media framework. Potentially, the media framework could be compromised by loading malicious links if there is a zero-day attack on the media framework. That's something we can't fix on the platform unless we stop loading arbitrary URLs in the media framework.

Comment 9

[Alastor Wu [:alwu]]

Thank Baku and Tim!

It seems that I have to implement something similar to [1], which would create a channel and load the image url via the channel. Then we can decode the image and to obtain the image bitmap [2] from the surface image. Now I think it would be better doing those steps inside each platform event source because each platform might requires different sizes of image, so they can check the image from MediaImage array to decide which image url they would like to load (metadata might contain multiple different size images)

On Android, we have already have image decoder [3] which can be used to get bitmap from an URL [4]. So I'm going to implement a helper class which could help SMTC get the bitmap image by providing an image URL.

However, as the MPRIS doesn't support sending a bitmap as an input, so we probably need to give up with setting artwork for MRPIS, and always use branding image (or favicon image) instead.

[1] https://searchfox.org/mozilla-central/rev/8526066f548af9ec3ebb462ff73c47ccc183f533/widget/android/ImageDecoderSupport.cpp#148-169
[2] https://searchfox.org/mozilla-central/source/widget/android/ImageDecoderSupport.cpp#40-45
[3] https://searchfox.org/mozilla-central/rev/8526066f548af9ec3ebb462ff73c47ccc183f533/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/ImageDecoder.java#30-45

Comment 10

[Alastor Wu [:alwu]]

Hi, Baku,

Sorry, I didn't understand this part you can obtain the branding icon as a resource. I know that we have already had branding images, eg, default128.png.

Therefore, I wonder if we have any way to get its absolute file-based URL directly? Because I feel that using a hardcode path like [1] is not a good way. And we have to ensure the path of image is not packed inside a jar file, which would require additional unzip.

Thank you.

[1] https://phabricator.services.mozilla.com/D67745

Comment 11

[Andrea Marchesini [:baku]]

You should be able to use the "XREAppDist" directory. See: https://developer.mozilla.org/en-US/docs/Extensions/Using_the_DOM_File_API_in_chrome_code#Accessing_files_in_a_special_directory

Comment 12

[Ilya K]

On Linux, MPRIS only accepts image URL that can be web-based protocal or file-based protocal

Just dropping by as someone who's worked with MPRIS before and is very interested in this working correctly:

I'm not sure your reading of the spec is correct - it just says:

URIs should be sent as (UTF-8) strings. Local files should use the "file://" schema.

Which doesn't explicitly limit the URI schemas to file:// or http://.

I've also tested this myself, and at least on KDE Plasma, data: URIs are accepted and displayed correctly.

Comment 13

[C.M.Chang[:chunmin]]

Hi Marc,

Are you still working on this bug? I could take this bug if you are not available to continue on this topic.

Comment 14

[Marc Streckfuß [:MeFisto94]]

Hey Chunmin,
I didn't work on this bug yet due to uncertainties on how to approach this correctly and waiting for the background changes/decisions.

Feel free to take it, though, since I am quite busy at the moment

Comment 15

[C.M.Chang[:chunmin]]

I plan to implement SMTC part only in this bug.

Comment 16

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P1: Replace `Maybe<const wchar_t*>` by `const wchar_t*`

• The maybe value could be nullptr even it's Some
• Nothing can be replaced by nullptr.

Comment 17

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P2: Delete comments

Depends on D77877

Comment 18

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P3: Return false when `SetMusicMetadata` fails

Depends on D77878

Comment 19

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P4: No need to set default metadata when initializing

Metadata would be reset every time when SetMetadata is called.

Depends on D77879

Comment 20

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P5: The arguments to `SetMusicMetadata` must be non-null

HStringRefernece must be constructed with a non-null wchar_t*. The
raw pointer returned from nsString::get() is a non-null address so
it's ok to add an assertion in SetMusicMetadata.

Depends on D77880

Comment 21

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P6: No need to update metadata upon opening SMTC

Depends on D77881

Comment 22

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P7: Rename `IMSTCDisplayUpdater` to `ISMTCDisplayUpdater`

Fix typo: MSTC should be SMTC.

Depends on D77882

Comment 23

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P8: Rework SMTP opening

• Add error messages so it's easier to debug when the error occurs
• It's better to unregister the event listener if failed to open SMTP.
  (In release, the key-event listener may be alive when the SMTP isn't
  initialized successfully)

Depends on D77883

Comment 24

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P9: Assert mControls instead of mInitialized

Depends on D77884

Comment 25

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P10: Assert mDisplay instead of mInitialized

Depends on D77885

Comment 26

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P11: Make methods used privately private

Some functions are listed as public methods but they are only used
privately. It's better to make them private.

Depends on D77886

Comment 27

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P12: Reorganize member functions

• Group related functions together
• Sync the function order in .cpp and .h (except destructor)
• Reword the comment for update

Depends on D77887

Comment 28

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P13: Apply the same format for comments

Depends on D77888

Comment 29

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P14: Replace `\` by `/` in `#include`

\ is a window style in file path but Mozilla uses / instead.

Depends on D77889

Comment 30

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P15: Move Maybe.h from .h to .cpp

Maybe is used in the .cpp file only.

Depends on D77890

Comment 31

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P16: Add a method to set image to SMTC thumbnail

Add a method to set an image to SMTC's thumbmail. Some test-only
functions are added as well to check the main functionalities. Those
test-only functions will be removed in the next patch.

Depends on D77891

Comment 32

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P17: Set media-session's MediaImage to the SMTC interface

This patch does the following things:

1. Update the FetchImageHelper: Decode an image from URL and then
   encode it into a specified format
2. Use FetchImageHelper to fetch the MediaImage defined in
   media-session
3. Upon the above image is fetched, set it to the SMTC's thumbnail

Depends on D77892

Comment 33

[C.M.Chang[:chunmin]]

Hi Andrew,

I was wondering if you know how to get the raw binary data of an image (bytes of the whole file that includes header) from an imgIContainer?

The intention of this bug is to set the image defined by an URL to the platform-level media control interface, on Windows. The example is here. The media control interface on Windows will pop up when the volume+/- key is pressed.

In the current implementation, I need to store the bytes of the whole image and then pass it to the underlying platform API. Now, once the image is fetched from a URL and return an imgIContainer from FetchImageHelper, the data within imgIContainer will be encoded to image file with a specified mime type (implementation in this patch). However, if the specified mime type is a PNG, and the image file specified by the URL is also PNG, the encoding process seems unnecessary. I was wondering if you happen to know how to get the raw binary data of an image file from imgIContainer.

Comment 34

[C.M.Chang[:chunmin]]

(In reply to C.M.Chang[:chunmin] from comment #33)
Hi Jessie,

I was wondering if you know who might have the insight into the problem mentioned in #comment33.

Comment 35

[Jessie [:jbonisteel] pls NI]

aosmond should be back soon to take a look, you can also try tnikkel.

Comment 36

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P18: Fetch next available image if fetching fails

Comment 37

[C.M.Chang[:chunmin]]

Attachment: Bug 1623971 - P19: Resolve fetching upon image is ready

The FetchImageHelp doesn't need to decode the fetched image before
handing the image to its caller since the decoded raw data won't be used
in the caller. The ImagePromise can be resolved upon image is fetched
(ready) instead.

Depends on D79222

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f34dd451242
P1: Replace `Maybe<const wchar_t*>` by `const wchar_t*` r=alwu
https://hg.mozilla.org/integration/autoland/rev/bb32e9a08fbc
P2: Delete comments r=alwu
https://hg.mozilla.org/integration/autoland/rev/6a1f5c915315
P3: Return false when `SetMusicMetadata` fails r=alwu
https://hg.mozilla.org/integration/autoland/rev/8e4699e03425
P4: No need to set default metadata when initializing r=alwu
https://hg.mozilla.org/integration/autoland/rev/caca22b92048
P5: The arguments to `SetMusicMetadata` must be non-null r=alwu
https://hg.mozilla.org/integration/autoland/rev/03b978f0a35b
P6: No need to update metadata upon opening SMTC r=alwu
https://hg.mozilla.org/integration/autoland/rev/603f37b78016
P7: Rename `IMSTCDisplayUpdater` to `ISMTCDisplayUpdater` r=alwu
https://hg.mozilla.org/integration/autoland/rev/2d2765846488
P8: Rework SMTP opening r=alwu
https://hg.mozilla.org/integration/autoland/rev/3c572c417823
P9: Assert mControls instead of mInitialized r=alwu
https://hg.mozilla.org/integration/autoland/rev/a52ca855c703
P10: Assert mDisplay instead of mInitialized r=alwu
https://hg.mozilla.org/integration/autoland/rev/ac69b774f094
P11: Make methods used privately private r=alwu
https://hg.mozilla.org/integration/autoland/rev/bba6f9948eaf
P12: Reorganize member functions r=alwu
https://hg.mozilla.org/integration/autoland/rev/bf18c71cc244
P13: Apply the same format for comments r=alwu
https://hg.mozilla.org/integration/autoland/rev/580624774939
P14: Replace `\` by `/` in `#include` r=alwu
https://hg.mozilla.org/integration/autoland/rev/ae2df4d87725
P15: Move Maybe.h from .h to .cpp r=alwu
https://hg.mozilla.org/integration/autoland/rev/2b7c4c1293c8
P16: Add a method to set image to SMTC thumbnail r=alwu,thomasmo
https://hg.mozilla.org/integration/autoland/rev/b57d9a939f73
P17: Set media-session's MediaImage to the SMTC interface r=alwu,thomasmo
https://hg.mozilla.org/integration/autoland/rev/03181031265b
P18: Fetch next available image if fetching fails r=alwu
https://hg.mozilla.org/integration/autoland/rev/aec24340c84f
P19: Resolve fetching upon image is ready r=alwu

Comment 39

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/8f34dd451242
https://hg.mozilla.org/mozilla-central/rev/bb32e9a08fbc
https://hg.mozilla.org/mozilla-central/rev/6a1f5c915315
https://hg.mozilla.org/mozilla-central/rev/8e4699e03425
https://hg.mozilla.org/mozilla-central/rev/caca22b92048
https://hg.mozilla.org/mozilla-central/rev/03b978f0a35b
https://hg.mozilla.org/mozilla-central/rev/603f37b78016
https://hg.mozilla.org/mozilla-central/rev/2d2765846488
https://hg.mozilla.org/mozilla-central/rev/3c572c417823
https://hg.mozilla.org/mozilla-central/rev/a52ca855c703
https://hg.mozilla.org/mozilla-central/rev/ac69b774f094
https://hg.mozilla.org/mozilla-central/rev/bba6f9948eaf
https://hg.mozilla.org/mozilla-central/rev/bf18c71cc244
https://hg.mozilla.org/mozilla-central/rev/580624774939
https://hg.mozilla.org/mozilla-central/rev/ae2df4d87725
https://hg.mozilla.org/mozilla-central/rev/2b7c4c1293c8
https://hg.mozilla.org/mozilla-central/rev/b57d9a939f73
https://hg.mozilla.org/mozilla-central/rev/03181031265b
https://hg.mozilla.org/mozilla-central/rev/aec24340c84f

Comment 40

[C.M.Chang[:chunmin]]

The NI is moved to https://bugzilla.mozilla.org/show_bug.cgi?id=1647511#c1