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

👁 Image	Alastor Wu [:alwu] Reporter
	Description • 6 years ago

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.

👁 Image	Alastor Wu [:alwu] Reporter
	Comment 1 • 6 years ago

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.

👁 Image	Marc Streckfuß [:MeFisto94]
	Comment 2 • 6 years ago

👁 Image	Tom S. (please needinfo tschuster)
	Comment 3 • 6 years ago

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,

👁 Image	Marc Streckfuß [:MeFisto94]
	Comment 4 • 6 years ago

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.

👁 Image	Marc Streckfuß [:MeFisto94]
	Comment 5 • 6 years ago

👁 Image	Alastor Wu [:alwu] Reporter
	Comment 6 • 6 years ago

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.

👁 Image	Andrea Marchesini [:baku]
	Comment 7 • 6 years ago

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.

👁 Image	Tim Huang[:timhuang]
	Comment 8 • 6 years ago

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.

👁 Image	Alastor Wu [:alwu] Reporter
	Comment 9 • 6 years ago

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

👁 Image	Alastor Wu [:alwu] Reporter
	Comment 10 • 6 years ago

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

👁 Image	Andrea Marchesini [:baku]
	Comment 11 • 6 years ago

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

👁 Image	Alastor Wu [:alwu] Reporter
	Updated • 6 years ago

👁 Image	Ilya K
	Comment 12 • 6 years ago

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.

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 13 • 6 years ago

Hi Marc,

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

👁 Image	Marc Streckfuß [:MeFisto94]
	Comment 14 • 6 years ago

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

👁 Image	C.M.Chang[:chunmin] Assignee
	Updated • 6 years ago

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 15 • 6 years ago

I plan to implement SMTC part only in this bug.

👁 Image	C.M.Chang[:chunmin] Assignee
	Updated • 6 years ago

👁 Image	C.M.Chang[:chunmin] Assignee
	Updated • 6 years ago

👁 Image	C.M.Chang[:chunmin] Assignee
	Updated • 6 years ago

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 16 • 6 years ago

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

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 17 • 6 years ago

Depends on D77877

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 18 • 6 years ago

Depends on D77878

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 19 • 6 years ago

Metadata would be reset every time when SetMetadata is called.

Depends on D77879

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 20 • 6 years ago

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

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 21 • 6 years ago

Depends on D77881

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 22 • 6 years ago

Fix typo: MSTC should be SMTC.

Depends on D77882

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 23 • 6 years ago

• 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

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 24 • 6 years ago

Depends on D77884

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 25 • 6 years ago

Depends on D77885

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 26 • 6 years ago

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

Depends on D77886

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 27 • 6 years ago

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

Depends on D77887

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 28 • 6 years ago

Depends on D77888

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 29 • 6 years ago

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

Depends on D77889

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 30 • 6 years ago

Maybe is used in the .cpp file only.

Depends on D77890

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 31 • 6 years ago

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

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 32 • 6 years ago

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

👁 Image	C.M.Chang[:chunmin] Assignee
	Updated • 6 years ago

👁 Image	C.M.Chang[:chunmin] Assignee
	Updated • 6 years ago

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 33 • 6 years ago • Edited

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.

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 34 • 6 years ago

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

👁 Image	Phabricator Automation
	Updated • 6 years ago

👁 Image	Phabricator Automation
	Updated • 6 years ago

👁 Image	Jessie [:jbonisteel] pls NI
	Comment 35 • 6 years ago

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

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 36 • 6 years ago

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 37 • 6 years ago

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

👁 Image	Pulsebot
	Comment 38 • 5 years ago

Pushed by cchang@mozilla.com: 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

👁 Image	Dorel Luca [:dluca]
	Comment 39 • 5 years ago
bugherder

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

👁 Image	Calixte Denizet (:calixte)
	Updated • 5 years ago

👁 Image	C.M.Chang[:chunmin] Assignee
	Updated • 5 years ago

👁 Image	C.M.Chang[:chunmin] Assignee
	Updated • 5 years ago

👁 Image	C.M.Chang[:chunmin] Assignee
	Comment 40 • 5 years ago

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

👁 Image	Phabricator Automation
	Updated • 5 years ago

👁 Image	Phabricator Automation
	Updated • 5 years ago

👁 Image	C.M.Chang[:chunmin] Assignee
	Updated • 4 years ago