Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1809943002:
Select BackingStrategy based on GpuPreferences (Closed)
|
Created:
4 years, 9 months ago by william.xie Modified:
4 years, 9 months ago CC:
DaleCurtis, chromium-reviews, darin-cc_chromium.org, jam, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSelect BackingStrategy based on GpuPreferences.
BUG=
Committed: https://crrev.com/c27b5e8077a8f42d931c41fa5201d972531c434b
Cr-Commit-Position: refs/heads/master@{#382780}
Patch Set 1 : #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Patch Set 5 : Rebase #
Messages
Total messages: 50 (20 generated)
Description was changed from ========== Propagate kEnableThreadedTextureMailboxes switch to the GPU process. GPU process is separated from browser process, now we need to propagate kEnableThreadedTextureMailboxes switch to the GPU process. BUG= ========== to ========== Propagate kEnableThreadedTextureMailboxes switch to the GPU process. GPU process is separated from browser process, now we need to propagate kEnableThreadedTextureMailboxes switch to the GPU process. BUG= ==========
william.xie@chromium.org changed reviewers: + piman@chromium.org
PTAL :)
Patchset #1 (id:1) has been deleted
This should already be propagated via GpuPreferences.enable_threaded_texture_mailboxes
On 2016/03/17 17:46:39, piman wrote: > This should already be propagated via > GpuPreferences.enable_threaded_texture_mailboxes From my test, set --enable-threaded-texture-mailboxes in command line doesn't work without this CL. can I know how to set it through command line parameter during launch chromium?
piman@chromium.org changed reviewers: + penghuang@chromium.org
+penghuang
On 2016/03/17 23:07:41, piman wrote: > +penghuang Looks like this switch is missing in gpu_host_messages.h, let me try to see if we can propagate it by GpuPreferences.
On 2016/03/17 23:32:21, william.xie wrote: > On 2016/03/17 23:07:41, piman wrote: > > +penghuang > > Looks like this switch is missing in gpu_host_messages.h, > let me try to see if we can propagate it by GpuPreferences. Sorry, got the switch in gpu_host_messages.h, however, in AndroidVideoDecodeAccelerator::UseDeferredRenderingStrategy(), we are trying to search this switch from GPU command line, actually this switch is not propagated to GPU command line because we are using GpuPreferences, so should we check GpuPreferences.enable_threaded_texture_mailboxes to get the switch value?
On 2016/03/17 23:37:56, william.xie wrote: > On 2016/03/17 23:32:21, william.xie wrote: > > On 2016/03/17 23:07:41, piman wrote: > > > +penghuang > > > > Looks like this switch is missing in gpu_host_messages.h, > > let me try to see if we can propagate it by GpuPreferences. > > Sorry, got the switch in gpu_host_messages.h, > however, in AndroidVideoDecodeAccelerator::UseDeferredRenderingStrategy(), > we are trying to search this switch from GPU command line, actually this switch > is not propagated to GPU command line because we are using GpuPreferences, so > should we check GpuPreferences.enable_threaded_texture_mailboxes to get the > switch value? Yes. We should use GpuPreferences.enable_threaded_texture_mailboxes.
On 2016/03/17 23:51:40, Peng wrote: > On 2016/03/17 23:37:56, william.xie wrote: > > On 2016/03/17 23:32:21, william.xie wrote: > > > On 2016/03/17 23:07:41, piman wrote: > > > > +penghuang > > > > > > Looks like this switch is missing in gpu_host_messages.h, > > > let me try to see if we can propagate it by GpuPreferences. > > > > Sorry, got the switch in gpu_host_messages.h, > > however, in AndroidVideoDecodeAccelerator::UseDeferredRenderingStrategy(), > > we are trying to search this switch from GPU command line, actually this > switch > > is not propagated to GPU command line because we are using GpuPreferences, so > > should we check GpuPreferences.enable_threaded_texture_mailboxes to get the > > switch value? > > Yes. We should use GpuPreferences.enable_threaded_texture_mailboxes. Interesting, I guess this has never worked then.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Hmm, I verified this worked in WebView when I added that check -- it seemed like the deferred path wasn't being used there. Has this changed recently?
On 2016/03/17 23:58:34, DaleCurtis wrote: > Hmm, I verified this worked in WebView when I added that check -- it seemed like > the deferred path wasn't being used there. Has this changed recently? there was a change recently to how gpu flags are propagated. i don't have the CL handy, will try to find. it definitely was working before.
On 2016/03/18 00:25:17, liberato wrote: > On 2016/03/17 23:58:34, DaleCurtis wrote: > > Hmm, I verified this worked in WebView when I added that check -- it seemed > like > > the deferred path wasn't being used there. Has this changed recently? > > there was a change recently to how gpu flags are propagated. i don't have the > CL handy, will try to find. > > it definitely was working before. Dear DaleCurtis@ watk@, In order to use GpuPreferences instead of command line check, how about passing GpuPreferences to VideoDecodeAccelerator by its Initialize function?
On Fri, Mar 18, 2016 at 3:22 AM, <william.xie@chromium.org> wrote: > On 2016/03/18 00:25:17, liberato wrote: > > On 2016/03/17 23:58:34, DaleCurtis wrote: > > > Hmm, I verified this worked in WebView when I added that check -- it > seemed > > like > > > the deferred path wasn't being used there. Has this changed recently? > > > > there was a change recently to how gpu flags are propagated. i don't > have the > > CL handy, will try to find. > > > > it definitely was working before. > > Dear DaleCurtis@ watk@, > In order to use GpuPreferences instead of command line check, how about > passing > GpuPreferences to VideoDecodeAccelerator by its Initialize function? > That is the common pattern (either the full GpuPreferences, or the indifidual bits that are needed). Antoine > > > > https://codereview.chromium.org/1809943002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
dalecurtis@chromium.org changed reviewers: + boliu@chromium.org
This function is used in GPU process startup while building the GPU info structure. I wrote it this way since little else is initialized by this point. Using GPU preferences is fine with me if we can get it here or refactor this code to set these values later during initialize. Note this likely means WebView is broken right now if it's included in Chrome experiments. So we either want to land a fix soon or revert the change that broke this. +boliu
On 2016/03/18 16:29:59, DaleCurtis wrote: > This function is used in GPU process startup while building the GPU info > structure. I wrote it this way since little else is initialized by this point. > > Using GPU preferences is fine with me if we can get it here or refactor this > code to set these values later during initialize. > > Note this likely means WebView is broken right now if it's included in Chrome > experiments. So we either want to land a fix soon or revert the change that > broke this. > > +boliu Webview never runs out of process gpu. And looks like the switch kEnableThreadedTextureMailboxes hasn't been removed yet, so this isn't broken in webview, not yet anyway.. Agree that this should be fixed by switching to checking GpuPreferences.enable_threaded_texture_mailboxes. (I would have hoped this was done already when GpuPreferences was added)
Ah, that's right, this isn't broken because it WebView only runs in-process-gpu, so the flag is always present when it's needed :)
Description was changed from ========== Propagate kEnableThreadedTextureMailboxes switch to the GPU process. GPU process is separated from browser process, now we need to propagate kEnableThreadedTextureMailboxes switch to the GPU process. BUG= ========== to ========== Propagate GpuPreferences to AVDA. BUG= ==========
Description was changed from ========== Propagate GpuPreferences to AVDA. BUG= ========== to ========== Propagate GpuPreferences to AVDA. This is required by BackingStrategy for now and maybe other features in the future. BUG= ==========
Description was changed from ========== Propagate GpuPreferences to AVDA. This is required by BackingStrategy for now and maybe other features in the future. BUG= ========== to ========== Propagate GpuPreferences to AVDA. This is required by BackingStrategy and maybe other features in the future. BUG= ==========
DaleCurtis@ PTAL
Patchset #2 (id:40001) has been deleted
dalecurtis@chromium.org changed reviewers: + liberato@chromium.org
+liberato who has a better idea of what we want here. https://codereview.chromium.org/1809943002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1809943002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:498: const gpu::GpuPreferences& gpu_preferences = Does this need to be passed in here or is this something AVDA can get itself. I think we're already requesting something similar within one of the shared state or strategy modules.
liberato@chromium.org changed reviewers: + posciak@chromium.org
i don't think that we don't need to pass GpuPreferences into the constructor, since |gl_decoder_|->GetContextGroup()->gpu_preferences() should provide it. +posciak, who is refactoring VDA initialization, to get his opinion on GetCapabilities (https://codereview.chromium.org/1745903002/) thanks -fl; https://codereview.chromium.org/1809943002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1809943002/diff/60001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1193: if (!gpu_preferences.enable_threaded_texture_mailboxes) { rather than have two copies of this check which must be kept in sync, maybe add preferences as a parameter to UseDeferredRenderingStrategy.
PTAL, Now we get the gpu_preferences from AVDA itself, but for GetCapabilities, we still need to pass gpu_preferences as its static attribute. https://codereview.chromium.org/1809943002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1809943002/diff/60001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1193: if (!gpu_preferences.enable_threaded_texture_mailboxes) { On 2016/03/21 16:39:52, liberato wrote: > rather than have two copies of this check which must be kept in sync, maybe add > preferences as a parameter to UseDeferredRenderingStrategy. Done. https://codereview.chromium.org/1809943002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/1809943002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:498: const gpu::GpuPreferences& gpu_preferences = On 2016/03/21 16:15:55, DaleCurtis wrote: > Does this need to be passed in here or is this something AVDA can get itself. I > think we're already requesting something similar within one of the shared state > or strategy modules. Done.
Description was changed from ========== Propagate GpuPreferences to AVDA. This is required by BackingStrategy and maybe other features in the future. BUG= ========== to ========== Select BackingStrategy based on GpuPreferences. BUG= ==========
lgtm
lgtm % one small nit. https://codereview.chromium.org/1809943002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1809943002/diff/80001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1160: const gpu::GpuPreferences& gpu_preferences = i was thinking that you might pass in the GpuPreferences here, and call it from both the constructor and GetCapabilities. otherwise, there's still two copies of the check.
ping DaleCurtis@ https://codereview.chromium.org/1809943002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1809943002/diff/80001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1160: const gpu::GpuPreferences& gpu_preferences = On 2016/03/22 21:22:08, liberato wrote: > i was thinking that you might pass in the GpuPreferences here, and call it from > both the constructor and GetCapabilities. otherwise, there's still two copies > of the check. Thanks Frank, per https://codereview.chromium.org/1809943002/#msg28, we should get GpuPreferences from AVDA itself. DaleCurtis@, how do you think?
https://codereview.chromium.org/1809943002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1809943002/diff/80001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1160: const gpu::GpuPreferences& gpu_preferences = On 2016/03/22 at 23:11:07, william.xie wrote: > On 2016/03/22 21:22:08, liberato wrote: > > i was thinking that you might pass in the GpuPreferences here, and call it from > > both the constructor and GetCapabilities. otherwise, there's still two copies > > of the check. > > Thanks Frank, per https://codereview.chromium.org/1809943002/#msg28, > we should get GpuPreferences from AVDA itself. > DaleCurtis@, how do you think? I believe Frank just means you should have UseDeferredRendereringStrategy() take gpu preferences as a const& parameter and then pass in the value via the code you have here. this allows you to keep the UseDeferred...() function call in the places its used today instead of having both check this preference.
Good idea, frank. PTAL https://codereview.chromium.org/1809943002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1809943002/diff/80001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1160: const gpu::GpuPreferences& gpu_preferences = On 2016/03/22 23:14:32, DaleCurtis wrote: > On 2016/03/22 at 23:11:07, william.xie wrote: > > On 2016/03/22 21:22:08, liberato wrote: > > > i was thinking that you might pass in the GpuPreferences here, and call it > from > > > both the constructor and GetCapabilities. otherwise, there's still two > copies > > > of the check. > > > > Thanks Frank, per https://codereview.chromium.org/1809943002/#msg28, > > we should get GpuPreferences from AVDA itself. > > DaleCurtis@, how do you think? > > I believe Frank just means you should have UseDeferredRendereringStrategy() take > gpu preferences as a const& parameter and then pass in the value via the code > you have here. this allows you to keep the UseDeferred...() function call in the > places its used today instead of having both check this preference. Done.
lgtm
The CQ bit was checked by william.xie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1809943002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809943002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809943002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by william.xie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, liberato@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1809943002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809943002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809943002/120001
Message was sent while issue was closed.
Description was changed from ========== Select BackingStrategy based on GpuPreferences. BUG= ========== to ========== Select BackingStrategy based on GpuPreferences. BUG= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Select BackingStrategy based on GpuPreferences. BUG= ========== to ========== Select BackingStrategy based on GpuPreferences. BUG= Committed: https://crrev.com/c27b5e8077a8f42d931c41fa5201d972531c434b Cr-Commit-Position: refs/heads/master@{#382780} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c27b5e8077a8f42d931c41fa5201d972531c434b Cr-Commit-Position: refs/heads/master@{#382780} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
