Just so we're clear, this also affects Widevine as well as OpenH264. I'm fine with this change, provided we keep the timeout there, as I don't think the code that uses OpenH264 can handle it having not been downloaded yet. The code that uses Widevine can. So we need to keep the timeout there. Jesup may also have an opinion on the download of OpenH264.

Comment on attachment 8863164 [details] [diff] [review] Defer the install and update check for the OpenH264 plugin until the browser is idle Ehsan, I know next to nothing about the OpenH264 update and install. It would be a good thing to get someone that has at least some understanding of how it works.

Spohl can probably review this or redirect the review appropriately.

Users of H.264 webrtc (such as CiscoSpark) have been taught they have to wait a minute or so in a new profile to use it; mostly this affects people testing. It hits some people installing firefox in order to use Spark, but not a lot. If they try to use H264 in webrtc before it's downloaded, it won't be available and if the other side requires it (such as Spark), the call will fail. This would be more of an issue if the check would be deferred for much longer periods than an minute; what's the range of times in the field until a check would happen? (I presume it would happen on the first idle point).

(In reply to Randell Jesup [:jesup] from comment #5) > Users of H.264 webrtc (such as CiscoSpark) have been taught they have to > wait a minute or so in a new profile to use it; mostly this affects people > testing. It hits some people installing firefox in order to use Spark, but > not a lot. Sorry I don't know what Spark is, so I don't understand this comment. > If they try to use H264 in webrtc before it's downloaded, it won't be > available and if the other side requires it (such as Spark), the call will > fail. This would be more of an issue if the check would be deferred for > much longer periods than an minute; what's the range of times in the field > until a check would happen? (I presume it would happen on the first idle > point). Until the browser is idle, there is no known period of time. Let me explain what this patch does. Right now we wait 1 minute after startup in order to run the code that installs the OpenH264 plugin or checks for an update to it (GMPInstallManager.simpleCheckAndInstall()). That's not a reliable way to install the plugin if the goal is to ensure that the plugin is present and updated at all times when the user wants to use it, and fixing that isn't the goal of this bug. That's also not acceptable since the user may be trying to use their browser at the 1 minute point in time when we decide that it's a good time to run this code, which this bug is trying to solve. If the former issue is a problem that needs to be addressed separately. But I'm not sure why the current code was written this way. FWIW that was done in bug 1009816.

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #6) > > If they try to use H264 in webrtc before it's downloaded, it won't be > > available and if the other side requires it (such as Spark), the call will > > fail. This would be more of an issue if the check would be deferred for > > much longer periods than an minute; what's the range of times in the field > > until a check would happen? (I presume it would happen on the first idle > > point). > > Until the browser is idle, there is no known period of time. One thing that could be changed here is to drop the timeout argument to requestIdleCallback. Again, since I don't know what the requirements of this code is, I don't know whether that would be an improvement or not...

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #6) > (In reply to Randell Jesup [:jesup] from comment #5) > > Users of H.264 webrtc (such as CiscoSpark) have been taught they have to > > wait a minute or so in a new profile to use it; mostly this affects people > > testing. It hits some people installing firefox in order to use Spark, but > > not a lot. > > Sorry I don't know what Spark is, so I don't understand this comment. Some Webrtc services are H264-only, because they also support H264-only (often hardware) endpoints (like various Cisco videoconferencing equipment). Services *really* don't want to transcode video, so they need a common codec. > > If they try to use H264 in webrtc before it's downloaded, it won't be > > available and if the other side requires it (such as Spark), the call will > > fail. This would be more of an issue if the check would be deferred for > > much longer periods than an minute; what's the range of times in the field > > until a check would happen? (I presume it would happen on the first idle > > point). > > Until the browser is idle, there is no known period of time. Right; I was hoping someone had Telemetry on where the first (reasonable?) idle point typically comes. > Let me explain what this patch does. Right now we wait 1 minute after > startup in order to run the code that installs the OpenH264 plugin or checks > for an update to it (GMPInstallManager.simpleCheckAndInstall()). That's not > a reliable way to install the plugin if the goal is to ensure that the > plugin is present and updated at all times when the user wants to use it, > and fixing that isn't the goal of this bug. That's also not acceptable > since the user may be trying to use their browser at the 1 minute point in > time when we decide that it's a good time to run this code, which this bug > is trying to solve. > > If the former issue is a problem that needs to be addressed separately. But > I'm not sure why the current code was written this way. FWIW that was done > in bug 1009816. It was done this way to avoid startup-time regressions or interfering with "start the browser with this URL" scenarios. It is certainly a heuristic guess at a less-bad-time than immediately on startup, and 'idle' would be better, though perhaps with a time limit on doing the check. If they open the browser and sit there, I'd far rather do the check then than wait a minute. But it it's "the user has interacted with the browser in the last minute", that would be a real problem - the check might be deferred a Long Time, or if the user tends to start, browse/interact, then immediately close (and some do), it may never happen (or not for a long while). This is worse for initial install than updates. Note that we probably would *not* want to block if it's installed and we're just checking for updates, since that could cause real problems. Even blocking on initial install can cause problems - let's say I want to make an appear.in call (default vp8 or vp9). If I haven't installed OpenH264 yet (new profile), and the server isn't available for some reason, we don't want to block making a vp8 or vp9 call - freezing the tab (perhaps for a long time) when you try to Call or Answer is bad for realtime communication. Note that we need to verify if H264 is available *before* offering or answering a call; we can't offer or agree to a codec we don't have. If we have H264, even if the check for updates hasn't been done, we can use the one we have to make an immediate call, so that also speaks to not blocking the way widevine might. > One thing that could be changed here is to drop the timeout argument to > requestIdleCallback. Again, since I don't know what the requirements of > this code is, I don't know whether that would be an improvement or not... You could drop it or at least might it a lot tighter than a minute, or use wait for (say) 10s idle with a max minute or two. Just translating "wait a minute" to "wait until idle for a minute" is not a good translation.

Comment on attachment 8863164 [details] [diff] [review] Defer the install and update check for the OpenH264 plugin until the browser is idle Review of attachment 8863164 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Chris Pearce (:cpearce) from comment #4) > Spohl can probably review this or redirect the review appropriately. The code itself is fine, but I'm not the right person to ask whether or not this is acceptable for these GMPs. Chris, I think you're in a better position to answer that. There is some discussion in bug 1009816 about how the initial delay of one minute after startup came to be, but I'm not sure if this is still current.

(In reply to Randell Jesup [:jesup] from comment #8) > > > If they try to use H264 in webrtc before it's downloaded, it won't be > > > available and if the other side requires it (such as Spark), the call will > > > fail. This would be more of an issue if the check would be deferred for > > > much longer periods than an minute; what's the range of times in the field > > > until a check would happen? (I presume it would happen on the first idle > > > point). > > > > Until the browser is idle, there is no known period of time. > > Right; I was hoping someone had Telemetry on where the first (reasonable?) > idle point typically comes. I don't think we have that information (and it would be totally dependent on how the user uses the system anyway so relying on it for something like software installation seems pretty weird.) > > Let me explain what this patch does. Right now we wait 1 minute after > > startup in order to run the code that installs the OpenH264 plugin or checks > > for an update to it (GMPInstallManager.simpleCheckAndInstall()). That's not > > a reliable way to install the plugin if the goal is to ensure that the > > plugin is present and updated at all times when the user wants to use it, > > and fixing that isn't the goal of this bug. That's also not acceptable > > since the user may be trying to use their browser at the 1 minute point in > > time when we decide that it's a good time to run this code, which this bug > > is trying to solve. > > > > If the former issue is a problem that needs to be addressed separately. But > > I'm not sure why the current code was written this way. FWIW that was done > > in bug 1009816. > > It was done this way to avoid startup-time regressions or interfering with > "start the browser with this URL" scenarios. It is certainly a heuristic > guess at a less-bad-time than immediately on startup, and 'idle' would be > better, though perhaps with a time limit on doing the check. If they open > the browser and sit there, I'd far rather do the check then than wait a > minute. requestIdleCallback() will do the right thing irrespective of when the user became idle, be it 1 second after the browser was launched or longer. > But it it's "the user has interacted with the browser in the last minute", > that would be a real problem - the check might be deferred a Long Time, or > if the user tends to start, browse/interact, then immediately close (and > some do), it may never happen (or not for a long while). This is worse for > initial install than updates. No, that's not what this function does. See https://developer.mozilla.org/en-US/docs/Web/API/Window/requestIdleCallback. > Note that we probably would *not* want to block if it's installed and we're > just checking for updates, since that could cause real problems. Even > blocking on initial install can cause problems - let's say I want to make an > appear.in call (default vp8 or vp9). If I haven't installed OpenH264 yet > (new profile), and the server isn't available for some reason, we don't want > to block making a vp8 or vp9 call - freezing the tab (perhaps for a long > time) when you try to Call or Answer is bad for realtime communication. > Note that we need to verify if H264 is available *before* offering or > answering a call; we can't offer or agree to a codec we don't have. If we > have H264, even if the check for updates hasn't been done, we can use the > one we have to make an immediate call, so that also speaks to not blocking > the way widevine might. FWIW _I_ think both the new and the old behavior are pretty bad for users who need to use the OpenH264 plugin before it gets installed. I suppose we have just been hoping that there won't be too many users who will get in that situation, and I don't think this patch will meaningfully change that situation, but it seems weird to rely on the exact timing of when the user goes to their first WebRTC site to give them a working connection. I just don't think this bug is the right place to fix that, and I certainly don't think I'm the right person to do that work. :-) If you have ideas on how to make things better here please file a follow-up, I'm not even familiar enough with things here to be able to suggest a meaningful improvement besides "install/update OpenH264 also when it's first needed" which seems obvious! > > One thing that could be changed here is to drop the timeout argument to > > requestIdleCallback. Again, since I don't know what the requirements of > > this code is, I don't know whether that would be an improvement or not... > > You could drop it or at least might it a lot tighter than a minute, or use > wait for (say) 10s idle with a max minute or two. Just translating "wait a > minute" to "wait until idle for a minute" is not a good translation. I honestly don't see how one of these alternatives is meaningfully better than the other. I'd be happy to do whatever the reviewer(s) make me do here!

Comment on attachment 8863164 [details] [diff] [review] Defer the install and update check for the OpenH264 plugin until the browser is idle Review of attachment 8863164 [details] [diff] [review]: ----------------------------------------------------------------- I discusses this with Jesup, and we agree the behaviour with this change will be no worse, and likely better than what we have currently.

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5c768a8c11 Defer the install and update check for the OpenH264 plugin until the browser is idle; r=cpearce,spohl