Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
855553003:
Enable PepperVideoDecoder3D (Closed)
|
Created:
5 years, 11 months ago by Sergey Ulanov Modified:
5 years, 11 months ago Reviewers:
Wez CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable PepperVideoRenderer3D
Once the problem in PPB_VideoDecode API implementation that causing
plugin crash is fixed PepperVideoRenderer3D can be enabled. This CL
enables it for Chrome 42 and above.
BUG=447403
Committed: https://crrev.com/630438e5d4b96fb9436e6e98426f86c3621d0986
Cr-Commit-Position: refs/heads/master@{#312898}
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 6
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 15 (4 generated)
sergeyu@chromium.org changed reviewers: + wez@chromium.org
Have we actually tracked down the issue w/ the VideoDecode API yet? https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.cc:639: // Use 3D renderer only in Chrome 42 and above. Suggest clarifying that that's because it's only stable in Chrome M42, and/or linking to the bug for that. https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.cc:641: if (data.GetInteger("chromeVersionMajor", &chrome_version) && Ick! Is there really no way for PPAPI to get at the Chrome version? :( Could we bump the VideoDecode API version instead, perhaps? https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.cc:649: LOG(ERROR) << "Using PPB_VideoDecode API for video decoding."; Seems strange to log a warning in the error case, and an error in the success case. Suggest having three LOG(ERROR)s here - one for success, one for failure, and one for version < M42.
On 2015/01/17 03:03:20, Wez wrote: > Have we actually tracked down the issue w/ the VideoDecode API yet? Yes, the pending fix is here: https://codereview.chromium.org/805193006/ . I'm going to land this CL only after that fix is landed.
https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.cc:639: // Use 3D renderer only in Chrome 42 and above. On 2015/01/17 03:03:20, Wez wrote: > Suggest clarifying that that's because it's only stable in Chrome M42, and/or > linking to the bug for that. Done. https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.cc:641: if (data.GetInteger("chromeVersionMajor", &chrome_version) && On 2015/01/17 03:03:20, Wez wrote: > Ick! Is there really no way for PPAPI to get at the Chrome version? :( AFAICT No :( > > Could we bump the VideoDecode API version instead, perhaps? I don't think it's worth it. It's just a bug in the software decode path that causes it to return incorrect decode_id. https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.cc:649: LOG(ERROR) << "Using PPB_VideoDecode API for video decoding."; On 2015/01/17 03:03:20, Wez wrote: > Seems strange to log a warning in the error case, and an error in the success > case. > > Suggest having three LOG(ERROR)s here - one for success, one for failure, and > one for version < M42. Done.
https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.cc:641: if (data.GetInteger("chromeVersionMajor", &chrome_version) && On 2015/01/21 00:04:58, Sergey Ulanov wrote: > On 2015/01/17 03:03:20, Wez wrote: > > Ick! Is there really no way for PPAPI to get at the Chrome version? :( > > AFAICT No :( > > > > > > Could we bump the VideoDecode API version instead, perhaps? > > I don't think it's worth it. It's just a bug in the software decode path that > causes it to return incorrect decode_id. All we're doing here is working around the lack of an API version bump by calling out to JS for the Chrome version; I suspect that the reason that there is no PPAPI for that is that it's preferred that we check for API versions. Basically the issue here is that the API went to stable w/out actually being stable. :P Could we move this to an explicit enableVideoDecodeRenderer message sent by the client when creating the plugin? That way we avoid polluting Connect() with a generic Chrome version field and risking that getting (ab)used for something else.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
On 2015/01/21 03:17:42, Wez wrote: > https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... > File remoting/client/plugin/chromoting_instance.cc (right): > > https://codereview.chromium.org/855553003/diff/1/remoting/client/plugin/chrom... > remoting/client/plugin/chromoting_instance.cc:641: if > (data.GetInteger("chromeVersionMajor", &chrome_version) && > On 2015/01/21 00:04:58, Sergey Ulanov wrote: > > On 2015/01/17 03:03:20, Wez wrote: > > > Ick! Is there really no way for PPAPI to get at the Chrome version? :( > > > > AFAICT No :( > > > > > > > > > > Could we bump the VideoDecode API version instead, perhaps? > > > > I don't think it's worth it. It's just a bug in the software decode path that > > causes it to return incorrect decode_id. > > All we're doing here is working around the lack of an API version bump by > calling out to JS for the Chrome version; I suspect that the reason that there > is no PPAPI for that is that it's preferred that we check for API versions. > > Basically the issue here is that the API went to stable w/out actually being > stable. :P > > Could we move this to an explicit enableVideoDecodeRenderer message sent by the > client when creating the plugin? That way we avoid polluting Connect() with a > generic Chrome version field and risking that getting (ab)used for something > else. Done.
LGTM w/ nits https://codereview.chromium.org/855553003/diff/100001/remoting/client/plugin/... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/855553003/diff/100001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.cc:640: // of Chrome. Still refer to the bug here; we'll want to remove this hack from the plugin once the bug is fixed and the app has been rolled. https://codereview.chromium.org/855553003/diff/100001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.cc:655: << chrome_version << ")"; You don't have chrome_version any more. :) Can we simplify all this logging by just logging "Initializing 3D renderer" and "Initializing 2D renderer" - we can tell from which of the messages we see printed whether we try 3D, and whether it fails. https://codereview.chromium.org/855553003/diff/100001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/client_plugin_impl.js (right): https://codereview.chromium.org/855553003/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_plugin_impl.js:653: // Currently PNaCl doesn't allow to get Chrome version in the plugin, so this s/PNaCl/PPAPI Suggest: "... doesn't provide a way for plugins to check the Chrome version."
https://codereview.chromium.org/855553003/diff/100001/remoting/client/plugin/... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/855553003/diff/100001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.cc:640: // of Chrome. On 2015/01/22 19:00:46, Wez wrote: > Still refer to the bug here; we'll want to remove this hack from the plugin once > the bug is fixed and the app has been rolled. Done. https://codereview.chromium.org/855553003/diff/100001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.cc:655: << chrome_version << ")"; On 2015/01/22 19:00:46, Wez wrote: > You don't have chrome_version any more. :) > > Can we simplify all this logging by just logging "Initializing 3D renderer" and > "Initializing 2D renderer" - we can tell from which of the messages we see > printed whether we try 3D, and whether it fails. Done. https://codereview.chromium.org/855553003/diff/100001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/client_plugin_impl.js (right): https://codereview.chromium.org/855553003/diff/100001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_plugin_impl.js:653: // Currently PNaCl doesn't allow to get Chrome version in the plugin, so this On 2015/01/22 19:00:46, Wez wrote: > s/PNaCl/PPAPI > > Suggest: "... doesn't provide a way for plugins to check the Chrome version." Done.
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/855553003/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/630438e5d4b96fb9436e6e98426f86c3621d0986 Cr-Commit-Position: refs/heads/master@{#312898} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
