As title say, and additionally, the way the test is currently done really needs improvement: a decoder is created, not initialized and assume that the hardware accelerated flag will be set anyway. Also, on mac it feeds a SPS that indicates a 64x64 video ; for which VT will never use a HW decoder.

It would be less misleading to just show true instead of false on Mac.

I'm a bit unsure in regards to what idl definition is best when it comes to returning a promise (as I didn't find an existing example of an attribute that would return a promise: only functions. Some functions returning a promise were defined as returning a jsval object (like the Telemetry module). Some others (the majority I've seen) returned instead a nsISupports object. I used the later because it's the simplest to implement without digging into how that idl stuff works :bz could you please advise on what is the best practice here? And in the idl, I see a uuid that has changed whenever the prototyping has changed. Am I supposed to change that? where do I get those generated, or do I come make things up? Thank you

Some machines will always use software decoding for a 64x64 videos.

Detecting if hardware decoding is available is an asynchronous operation/ The use of Promise allowes the value displayed to be accurate on all platforms and not just windows.

Comment on attachment 8711642 [details] [diff] [review] P3. Use promise with supportsHardwareH264Decoding. r=cpearce The first P1 is really P2 sorry, forgot to upload the first patch first

Comment on attachment 8711615 [details] [diff] [review] P1. Have IsVideoAccelerated return a Promise. Review of attachment 8711615 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDOMWindowUtils.cpp @@ +2150,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsDOMWindowUtils::GetSupportsHardwareH264Decoding(nsISupports* *aPromise) nsISupports** aPromise @@ +2171,5 @@ > + RefPtr<Promise> promise = Promise::Create(parentObject, rv); > + if (rv.Failed()) { > + return NS_ERROR_FAILURE; > + } > + rv.ThrowDOMException(NS_ERROR_DOM_NOT_SUPPORTED_ERR, I'm not sure you should be both throwing and rejecting here. bz would know. ::: dom/interfaces/base/nsIDOMWindowUtils.idl @@ +48,5 @@ > interface nsIJSRAIIHelper; > interface nsIContentPermissionRequest; > interface nsIObserver; > > +// JYA: Do I need to update this? In the thread at https://groups.google.com/forum/#!searchin/mozilla.dev.platform/uuid/mozilla.dev.platform/n6qBpyxoI6I/4qxwFvBOAgAJ no one opposed not updating UUIDs...

Comment on attachment 8711641 [details] [diff] [review] P1. Use a 640x360 SPS to test for HW decoding support. Review of attachment 8711641 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/fmp4/MP4Decoder.cpp @@ +182,5 @@ > return Preferences::GetBool("media.mp4.enabled"); > } > > static const uint8_t sTestH264ExtraData[] = { > + 0x01, 0x42, 0xc0, 0x1e, 0xff, 0xe1, 0x00, 0x17, 0x67, 0x42, We should have a comment here explaining what these bytes are and more importantly why we had to chose the bytes we did (i.e. because some decoders don't HW accelerate 64x64 videos).

Comment on attachment 8711642 [details] [diff] [review] P3. Use promise with supportsHardwareH264Decoding. r=cpearce Review of attachment 8711642 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, but I don't think I'm qualified to r+ code here. Felipe Gomes r+'d the changes to this file in bug 1151611, so bumping to him... ::: toolkit/modules/Troubleshoot.jsm @@ +320,5 @@ > > data.numTotalWindows = 0; > data.numAcceleratedWindows = 0; > let winEnumer = Services.ww.getWindowEnumerator(); > + var seenH264 = false; Can that be `let` rather than `var`?

(In reply to Chris Pearce (:cpearce) from comment #6) > Comment on attachment 8711615 [details] [diff] [review] > P1. Have IsVideoAccelerated return a Promise. > > Review of attachment 8711615 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/base/nsDOMWindowUtils.cpp > @@ +2150,5 @@ > > return NS_OK; > > } > > > > NS_IMETHODIMP > > +nsDOMWindowUtils::GetSupportsHardwareH264Decoding(nsISupports* *aPromise) > > nsISupports** aPromise > I made it consistent with earlier (and later) declarations in this file. > @@ +2171,5 @@ > > + RefPtr<Promise> promise = Promise::Create(parentObject, rv); > > + if (rv.Failed()) { > > + return NS_ERROR_FAILURE; > > + } > > + rv.ThrowDOMException(NS_ERROR_DOM_NOT_SUPPORTED_ERR, > > I'm not sure you should be both throwing and rejecting here. bz would know. > I'm guessing the former will never happen while the 2nd could (not with our default compile options though) Rejecting the promise allow the about:support page to display the right info. > ::: dom/interfaces/base/nsIDOMWindowUtils.idl > @@ +48,5 @@ > > interface nsIJSRAIIHelper; > > interface nsIContentPermissionRequest; > > interface nsIObserver; > > > > +// JYA: Do I need to update this? > > In the thread at > https://groups.google.com/forum/#!searchin/mozilla.dev.platform/uuid/mozilla. > dev.platform/n6qBpyxoI6I/4qxwFvBOAgAJ no one opposed not updating UUIDs... Thanks for that.

Comment on attachment 8711642 [details] [diff] [review] P3. Use promise with supportsHardwareH264Decoding. r=cpearce Review of attachment 8711642 [details] [diff] [review]: ----------------------------------------------------------------- A lot of about:support displays localized info. This one currently doesn't, but since you're touching it, it would be nice to have. The graphics section makes it very easy to do. ::: toolkit/modules/Troubleshoot.jsm @@ +320,5 @@ > > data.numTotalWindows = 0; > data.numAcceleratedWindows = 0; > let winEnumer = Services.ww.getWindowEnumerator(); > + var seenH264 = false; yeah, use let please @@ +334,5 @@ > data.numTotalWindows++; > data.windowLayerManagerType = winUtils.layerManagerType; > data.windowLayerManagerRemote = winUtils.layerManagerRemote; > + if (!seenH264) { > + data.supportsHardwareH264 = "initializing"; s/initializing/Unknown/ @@ +335,5 @@ > data.windowLayerManagerType = winUtils.layerManagerType; > data.windowLayerManagerRemote = winUtils.layerManagerRemote; > + if (!seenH264) { > + data.supportsHardwareH264 = "initializing"; > + winUtils.supportsHardwareH264Decoding.then(function() { It's strange that the promise throws if the answer is "No". It's more common to resolve it with a boolean or status code, and only leave the error handler for errors. @@ +339,5 @@ > + winUtils.supportsHardwareH264Decoding.then(function() { > + data.supportsHardwareH264 = "Yes"; > + done(data); > + }, function(error) { > + data.supportsHardwareH264 = "No (" + error.message + ")"; the string would show up as "No (No; Compiled without MP4 support.)". Please make this better. @@ +361,5 @@ > } > > if (!gfxInfo) { > + if (!seenH264) { > + done(data); The control flow of this function is getting complicated, as this assumes that the only async operation here is the promise that you're adding. If someone adds a second promise to be resolved and overlooks the existing one there will be a race introduced and done can be called before the other promise has finished. The easiest thing to do is to make this function into an async task (see usages of `Task.async`), and just yield while your promise is running. ::: toolkit/modules/tests/browser/browser_Troubleshoot.js @@ +221,5 @@ > windowLayerManagerRemote: { > type: "boolean", > }, > supportsHardwareH264: { > + type: "Promise", this doesn't look correct. When your promise is resolved you're still assigning a string to this variable.

Comment on attachment 8711615 [details] [diff] [review] P1. Have IsVideoAccelerated return a Promise. > I'm a bit unsure in regards to what idl definition is best when > it comes to returning a promise Funny you should ask. Yesterday I would have told you that either jsval or nsISupports would be fine, but today, I will claim jsval is better because chances are as we transition to SpiderMonkey promises we will need to change all the nsISupports things to jsval instead. Method vs attribute shouldn't matter for this. Given a Promise*, returning it as a jsval is pretty simple: rval.setObject(*promise->GetWrapper()); As far as the patch goes, this bit: >+ if (rv.Failed()) { >+ return NS_ERROR_FAILURE; should return rv.StealNSResult(). Otherwise you will hit a MOZ_ASSERT if rv is in fact failed there. Which will only happen on JS engine OOM, anyway... Both throwing and rejecting is in fact the right thing here: ThrowDOMException on rv puts a pending exception on it, and then rejecting the promise with rv transfers that exception to the Promise as a rejection value. That said, it should be promise->MaybeReject(rv), no? The other won't compile. For the uuid, you can probably just leave it, but if you do want to create a new one, the simplest way is "/msg firebot uuid" on IRC.

> because chances are as we transition to SpiderMonkey promises we will need to change all the nsISupports > things to jsval instead Turns out that's not necessarily true, but still pretty likely, so I really would stick to jsval if it's not a problem.

(In reply to Boris Zbarsky [:bz] from comment #11) > Comment on attachment 8711615 [details] [diff] [review] > P1. Have IsVideoAccelerated return a Promise. > > > I'm a bit unsure in regards to what idl definition is best when > > it comes to returning a promise > > Funny you should ask. Yesterday I would have told you that either jsval or > nsISupports would be fine, but today, I will claim jsval is better because > chances are as we transition to SpiderMonkey promises we will need to change > all the nsISupports things to jsval instead. > > Method vs attribute shouldn't matter for this. > > Given a Promise*, returning it as a jsval is pretty simple: > > rval.setObject(*promise->GetWrapper()); > > As far as the patch goes, this bit: > > >+ if (rv.Failed()) { > >+ return NS_ERROR_FAILURE; > > should return rv.StealNSResult(). Otherwise you will hit a MOZ_ASSERT if rv > is in fact failed there. Which will only happen on JS engine OOM, anyway... Great thank you... will use jsval. There's some inconsistencies on how to return the jsval thorough our code... Thanks for showing me GetWrapper() method. I did think that seeing the code to use jsval appeared more complicated, it must have been the right way to proceed :)

(In reply to :Felipe Gomes (needinfo me!) from comment #10) > Comment on attachment 8711642 [details] [diff] [review] > P3. Use promise with supportsHardwareH264Decoding. r=cpearce > > Review of attachment 8711642 [details] [diff] [review]: > ----------------------------------------------------------------- > > A lot of about:support displays localized info. This one currently doesn't, > but since you're touching it, it would be nice to have. The graphics section > makes it very easy to do. I have considered that, and would be happy to open a following bug for it. I feel that this is out of scope for the exercise of this bug. One of my concern with translation however, is that the message we return is of the OS when it failed to initialise hardware decoding. This is not something we want to translate, seeing that it will already be in localised language in most cases. Adding that this string is only of for the graphic team who are all English speaking ; adding an extra layer of translation will only make debugging harder. This string was only added because the graphic team needed hints on why HW decoding was disabled. The other issue at hand with translating is that the strings are translated *after* returning from Troubleshoot.jsm ; and here we are building a string from two strings... This would move the work of translating to Troubleshoot.jsm which currently has no mechanism in place to do it. > > ::: toolkit/modules/Troubleshoot.jsm > @@ +320,5 @@ > > > > data.numTotalWindows = 0; > > data.numAcceleratedWindows = 0; > > let winEnumer = Services.ww.getWindowEnumerator(); > > + var seenH264 = false; > > yeah, use let please I learn JS as I go here :) didn't even know let until know. > > @@ +334,5 @@ > > data.numTotalWindows++; > > data.windowLayerManagerType = winUtils.layerManagerType; > > data.windowLayerManagerRemote = winUtils.layerManagerRemote; > > + if (!seenH264) { > > + data.supportsHardwareH264 = "initializing"; > > s/initializing/Unknown/ this string is really never ever used. My original goal was to have the value of the string updated *after* the page had already been rendered. So "initalizing" would have been displayed temporarily while a secondary thread was completing his run and attempting to initialize the hardware. > > @@ +335,5 @@ > > data.windowLayerManagerType = winUtils.layerManagerType; > > data.windowLayerManagerRemote = winUtils.layerManagerRemote; > > + if (!seenH264) { > > + data.supportsHardwareH264 = "initializing"; > > + winUtils.supportsHardwareH264Decoding.then(function() { > > It's strange that the promise throws if the answer is "No". It's more common > to resolve it with a boolean or status code, and only leave the error > handler for errors. This is how the media promise works. If initialization of the hardware has completed, the promise has been resolved. If not, it is rejected with the error message. So a promise rejected means that HW decoding is not available, and the message indicates on why it wasn't available. This is not something you can do with a status code, especially as this status code will be dependent on the underlying OS, OS version, locale set etc... All of this is only for debugging purposes and troubleshooting. It wasn't working on anything but Windows until this bug. We've had several bug reports of people telling us that HW decoding wasn't working, only going by the value returned by this screen. This takes time and resources to troubleshoot and confuse the user. > > @@ +339,5 @@ > > + winUtils.supportsHardwareH264Decoding.then(function() { > > + data.supportsHardwareH264 = "Yes"; > > + done(data); > > + }, function(error) { > > + data.supportsHardwareH264 = "No (" + error.message + ")"; > > the string would show up as "No (No; Compiled without MP4 support.)". > Please make this better. I will. However, there is now no case where this would ever happen as mp4 support is always enabled. > > @@ +361,5 @@ > > } > > > > if (!gfxInfo) { > > + if (!seenH264) { > > + done(data); > > The control flow of this function is getting complicated, as this assumes > that the only async operation here is the promise that you're adding. If > someone adds a second promise to be resolved and overlooks the existing one > there will be a race introduced and done can be called before the other > promise has finished. I agree that what is there now is less than ideal. I can change that to use Promise.All instead, making future changes easier. > > The easiest thing to do is to make this function into an async task (see > usages of `Task.async`), and just yield while your promise is running. > not sure how this would help here. It would be really nice to have a mechanism where a string is defined as having a "future" value. So we can set it to whatever initial value (unknown, initializing whatever) and it gets updated later automatically. > ::: toolkit/modules/tests/browser/browser_Troubleshoot.js > @@ +221,5 @@ > > windowLayerManagerRemote: { > > type: "boolean", > > }, > > supportsHardwareH264: { > > + type: "Promise", > > this doesn't look correct. When your promise is resolved you're still > assigning a string to this variable. ok.. I wasn't sure on what that was for.

Some machines will always use software decoding for a 64x64 videos. Updated comments. Carrying r+

Use jsval instead. Carrying r+

(In reply to :Felipe Gomes (needinfo me!) from comment #10) > Comment on attachment 8711642 [details] [diff] [review] > P3. Use promise with supportsHardwareH264Decoding. r=cpearce > > Review of attachment 8711642 [details] [diff] [review]: > ----------------------------------------------------------------- > > A lot of about:support displays localized info. This one currently doesn't, > but since you're touching it, it would be nice to have. The graphics section > makes it very easy to do. Matt Woodrow indicated that he wants all localisation to be removed from about:support instead. I have opened bug 1243185 in the mean time (which will likely be closed as WONTFIX)

Following discussion on IRC, the promise is now always resolved with the message (including Yes or No; $reason). I had separated the case to provide more flexibility to the JS on how things would be displayed. But I guess it doesn't matter.

Detecting if hardware decoding is available is an asynchronous operation/ The use of Promise allowes the value displayed to be accurate on all platforms and not just windows. Taking all comments into consideration. First the promise is now always resolved with the message to use. Exactly in effect what it used to do. I use a Promise.all so if anyone wants to add a promise it just need to append it to this array. Much more elegant approach, thank you for stirring me into that direction.

Comment on attachment 8712482 [details] [diff] [review] P3. Use promise with supportsHardwareH264Decoding. Review of attachment 8712482 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. About the removal of localized strings, I know it's not part of this bug, so let's leave that separate. But please ping me or people from SUMO before doing that, because about:support is the user-facing troubleshooting page, so I don't agree with removing that. ::: toolkit/modules/Troubleshoot.jsm @@ +339,5 @@ > } > data.numTotalWindows++; > data.windowLayerManagerType = winUtils.layerManagerType; > data.windowLayerManagerRemote = winUtils.layerManagerRemote; > + if (promises.length < 1) { why check for promises.length < 1? r+ with this removed

(In reply to :Felipe Gomes (needinfo me!) from comment #20) > Comment on attachment 8712482 [details] [diff] [review] > P3. Use promise with supportsHardwareH264Decoding. > > Review of attachment 8712482 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks. About the removal of localized strings, I know it's not part of this > bug, so let's leave that separate. But please ping me or people from SUMO > before doing that, because about:support is the user-facing troubleshooting > page, so I don't agree with removing that. > > ::: toolkit/modules/Troubleshoot.jsm > @@ +339,5 @@ > > } > > data.numTotalWindows++; > > data.windowLayerManagerType = winUtils.layerManagerType; > > data.windowLayerManagerRemote = winUtils.layerManagerRemote; > > + if (promises.length < 1) { > > why check for promises.length < 1? r+ with this removed because here we loop to check how many windows we have and print how many are hardware accelerated. The check for h264 decoding is as such performed as many times as you have Windows. It's totally unecessary, we only need to do it once. Additionally, seeing that you have a set limit of hardware decoder, looping through windows could actually provide wrong results. It was a bug in the original code that I fixed while at it.

Alright, just add a comment explaining that

If you have some windows that are hardware accelerated and some that aren't, will the answer depend on whichever window comes first in the loop?

(In reply to :Felipe Gomes (needinfo me!) from comment #23) > If you have some windows that are hardware accelerated and some that aren't, > will the answer depend on whichever window comes first in the loop? No it's totally separated. While its displayed in the same section of about:support h264 is done in a completely separated process. It was less an issue before because we created the decoder on the main thread, read the value synchronously and shut it down immediately. But now we create a separate thread to run that operation. What we want to know is if you have a h264 hardware decoder available. The information is valid at the time presented. There's no guarantee that during playback it will actually be used

I'll see if it's possible to get that part of the loop altogether.

You can use `Services.wm.getMostRecentWindow("")` to get any window. If it needs to be a browser.xul for any reason, `Services.wm.getMostRecentWindow("navigator:browser")`

(In reply to :Felipe Gomes (needinfo me!) from comment #26) > You can use `Services.wm.getMostRecentWindow("")` to get any window. If it > needs to be a browser.xul for any reason, > `Services.wm.getMostRecentWindow("navigator:browser")` I have no idea what a browser.xul is :) is that for e10s the difference between the core process and the content one ?

No, that just differentiates the main browser window, from other windows such as dialogs, some popups, etc..

any window should do... I'll see what works.

https://hg.mozilla.org/integration/mozilla-inbound/rev/228267494daa https://hg.mozilla.org/integration/mozilla-inbound/rev/bd7599dcee0d https://hg.mozilla.org/integration/mozilla-inbound/rev/7215ed1cfcef

https://hg.mozilla.org/mozilla-central/rev/228267494daa https://hg.mozilla.org/mozilla-central/rev/bd7599dcee0d https://hg.mozilla.org/mozilla-central/rev/7215ed1cfcef

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/08eddf3af531 for WinXP debug crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=20741651&repo=mozilla-inbound

So this made it to central or it did not?

(In reply to Jean-Yves Avenard [:jya] from comment #33) > So this made it to central or it did not? The original patches made it to central. Then I backed them out on inbound, not realizing that these had already made it to central. Then I merged the backout to central. Our bug marking tool must not have commented on the merge to central.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c8b039b6d07 https://hg.mozilla.org/integration/mozilla-inbound/rev/381af1a45d10 https://hg.mozilla.org/integration/mozilla-inbound/rev/4891238709e1

https://hg.mozilla.org/mozilla-central/rev/6c8b039b6d07 https://hg.mozilla.org/mozilla-central/rev/381af1a45d10 https://hg.mozilla.org/mozilla-central/rev/4891238709e1