Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1505343003:
Updating texture validation to account for sampler objects in ES3 (Closed)
DescriptionUpdating texture validation to account for sampler objects in ES3
BUG=563613
Committed: https://crrev.com/838ca1836eae1357a3b9135e808266d7d28bd3fa
Cr-Commit-Position: refs/heads/master@{#371062}
Committed: https://crrev.com/7fd547659babda857da554a4d4514df0779aeb74
Cr-Commit-Position: refs/heads/master@{#372187}
Patch Set 1 #Patch Set 2 : Fixed compile issue in tests #Patch Set 3 : Fixed DCHECK that was causing bots to fail #
Total comments: 2
Patch Set 4 : Simplifying validation flow as suggested by zmo@ #Patch Set 5 : Fixed a couple of unnecessary changes #
Total comments: 6
Patch Set 6 : Addressed most of piman@'s feedback. #
Total comments: 6
Patch Set 7 : Rebase #
Total comments: 2
Patch Set 8 : Removed early out on !HasImages() #Patch Set 9 : Fixed unittest that was checking the wrong value #Patch Set 10 : Fixed issue on devices without TEXTURE_FLOAT_LINEAR #
Messages
Total messages: 60 (20 generated)
The CQ bit was checked by bajones@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1505343003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1505343003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by bajones@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1505343003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1505343003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by bajones@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1505343003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1505343003/40001
bajones@chromium.org changed reviewers: + kbr@chromium.org, piman@chromium.org, zmo@chromium.org
I feel like this could be a bit better architected, but I was trying to get away with the minimal amount of disruption possible. This patch does get https://www.khronos.org/registry/webgl/sdk/tests/conformance2/samplers/sample... passing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1505343003/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/1505343003/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.cc:301: Texture::Texture(GLuint service_id, bool is_es3_enabled) Per our offline discussion, I suggest we don't add es2/es3 state per texture, as it is conceptually incorrect (textures may be shared between ES2/ES3 contexts through mailbox). To make code simpler, I suggest we only update cube_completeness and mipmap_completeness whenever we update a texture, and leave all the can_render state at render time (I think it's really trivia once we have the cube/mipmap completeness already computed and cached) https://codereview.chromium.org/1505343003/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.cc:1130: } else if (!is_es3_enabled_) { Shouldn't we still check the following in ES3?
On 2015/12/11 20:18:54, Zhenyao Mo wrote: > Per our offline discussion, I suggest we don't add es2/es3 state per texture, as > it is conceptually incorrect (textures may be shared between ES2/ES3 contexts > through mailbox). Good point. That state is removed now. > To make code simpler, I suggest we only update cube_completeness and > mipmap_completeness whenever we update a texture, and leave all the can_render > state at render time (I think it's really trivia once we have the cube/mipmap > completeness already computed and cached) Agreed! Latest revision has a much simpler (IMO) validation flow.
https://codereview.chromium.org/1505343003/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (left): https://codereview.chromium.org/1505343003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:7659: if (!texture_manager()->HaveUnrenderableTextures() && This early exit is pretty important for performance, since this is called for every single draw call - in the general case where the application correctly defines its textures, this used to early exit. If not, we have to iterate over every single texture unit, of which there can be a lot (e.g. NVIDIA). How can we keep that property? https://codereview.chromium.org/1505343003/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/sampler_manager.h (right): https://codereview.chromium.org/1505343003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/sampler_manager.h:47: const SamplerState* sampler_state() const { nit: const SamplerState& https://codereview.chromium.org/1505343003/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/1505343003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.cc:68: SamplerState sampler_state, nit: const SamplerState& https://codereview.chromium.org/1505343003/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager.h (right): https://codereview.chromium.org/1505343003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.h:61: const SamplerState* sampler_state() const { nit: const SamplerState& https://codereview.chromium.org/1505343003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager.h:356: const SamplerState* sampler_state) const; nit: use const &
https://codereview.chromium.org/1505343003/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (left): https://codereview.chromium.org/1505343003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:7659: if (!texture_manager()->HaveUnrenderableTextures() && On 2015/12/14 14:04:14, piman (in KST timezone) wrote: > This early exit is pretty important for performance, since this is called for > every single draw call - in the general case where the application correctly > defines its textures, this used to early exit. If not, we have to iterate over > every single texture unit, of which there can be a lot (e.g. NVIDIA). > > How can we keep that property? For ES3 we can't evaluate this ahead of time because it depends on both the textures _and_ the samplers bound. We could try and update the value any time a texture or sampler binding changes, but that would probably be nearly as much or more work than simply checking it here at draw time. At least here we only have to check the texture units used by the current shader program. (Not for every unit supported by the hardware, as you feared.) A previous iteration of this patch attempted to keep this check for ES2 contexts and bypassed it for ES3, but that ended up tracking whether a texture was created with an ES2 context or a ES3 context, which zmo@ pointed out was faulty logic because said textures may be shared between contexts via mailbox. After some discussion we felt that the additional validation overhead here was small enough that it was worth the reduced code complexity. (We're also hoping to simply defer this logic to ANGLE entirely in the near future.) I'm happy to hear any alternative suggestions you have, though! I do believe there's room for improvement here.
Addressed the rest of piman@'s feedback in the latest patch.
https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7663: if (!texture_manager()->HaveImages()) { This is wrong. Now we can't short cut here as we don't know unrenderable texture count. https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:463: if (static_cast<size_t>(base_level_) >= face_infos_[0].level_infos.size()) { You should be able to DCHECK here because this case should lead to CAN_RENDER_NEVER.
https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7663: if (!texture_manager()->HaveImages()) { On 2015/12/14 22:48:33, Zhenyao Mo wrote: > This is wrong. Now we can't short cut here as we don't know unrenderable > texture count. Could this be updated so that at least ES 2.0 contexts can still short-cut these checks if there aren't any unrenderable textures in the texture manager? https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.h (right): https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.h:244: // Condition on which this texture is renderable. Can be ONLY_IF_NPOT if it Comment needs to be updated.
https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7663: if (!texture_manager()->HaveImages()) { On 2015/12/14 23:22:44, Ken Russell wrote: > On 2015/12/14 22:48:33, Zhenyao Mo wrote: > > This is wrong. Now we can't short cut here as we don't know unrenderable > > texture count. > > Could this be updated so that at least ES 2.0 contexts can still short-cut these > checks if there aren't any unrenderable textures in the texture manager? Now that we could possibly share textures among ES2/ES3 context through mailbox, we really can't decide until render time. Unless we have RENDERABLE_ES2 and HaveUnrenderableTexturesES2(), but then all the designs to unify ES2/ES3 texture handling will be lost. Assuming the ultimate step is to upgrade the compositor to also ES3, this shortcut is short-lived anyway. What do you think?
https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7663: if (!texture_manager()->HaveImages()) { On 2015/12/14 23:58:17, Zhenyao Mo wrote: > On 2015/12/14 23:22:44, Ken Russell wrote: > > On 2015/12/14 22:48:33, Zhenyao Mo wrote: > > > This is wrong. Now we can't short cut here as we don't know unrenderable > > > texture count. > > > > Could this be updated so that at least ES 2.0 contexts can still short-cut > these > > checks if there aren't any unrenderable textures in the texture manager? > > Now that we could possibly share textures among ES2/ES3 context through mailbox, > we really can't decide until render time. Unless we have RENDERABLE_ES2 and > HaveUnrenderableTexturesES2(), but then all the designs to unify ES2/ES3 texture > handling will be lost. Assuming the ultimate step is to upgrade the compositor > to also ES3, this shortcut is short-lived anyway. > > What do you think? It's a good point. As long as these checks are only being done for the textures consumed by the active program, I guess it sounds OK to remove the short-circuit. We need to be careful to not regress per-draw-call performance.
Ideally this validation would be done on binds and changes, rather than
draws ("pay for what you use"). It may require a little bit more tracking.
1- for the common case where samplers aren't used (es2, and virtually all
existing clients), keep track of the status assuming the Texture's sampler
state is used
2- on a per-unit basis (in the context), keep track of the status
corresponding to the texture/sampler combination. When the Texture is bound
and the current sampler is 0, use the Texture's validation state. When the
texture is bound and the current sampler is !0, or when a sampler is bound,
revalidate using the combination of Texture+Sampler.
3- when a Texture is changed, revalidate all units in the current context
where it is bound (including the active one). There can be some
optimizations there if needed (e.g. keep track on a per-Texture basis of
how many units it's bound to, across all contexts. Only revalidate all
units beyond the active one if the Texture is bound to >1 unit, which will
be uncommon). Could do it lazily (keep a dirty bit) to only revalidate once
over a series of TexParameter changes (common at initialization time).
4- when a Sampler is changed, same.
5- on a context switch, revalidate all units (if needed we could keep a
version number on the TextureManager to avoid that).
On Tue, Dec 15, 2015 at 9:27 AM, <kbr@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
>
>
https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:7663: if
> (!texture_manager()->HaveImages()) {
> On 2015/12/14 23:58:17, Zhenyao Mo wrote:
>
>> On 2015/12/14 23:22:44, Ken Russell wrote:
>> > On 2015/12/14 22:48:33, Zhenyao Mo wrote:
>> > > This is wrong. Now we can't short cut here as we don't know
>>
> unrenderable
>
>> > > texture count.
>> >
>> > Could this be updated so that at least ES 2.0 contexts can still
>>
> short-cut
>
>> these
>> > checks if there aren't any unrenderable textures in the texture
>>
> manager?
>
> Now that we could possibly share textures among ES2/ES3 context
>>
> through mailbox,
>
>> we really can't decide until render time. Unless we have
>>
> RENDERABLE_ES2 and
>
>> HaveUnrenderableTexturesES2(), but then all the designs to unify
>>
> ES2/ES3 texture
>
>> handling will be lost. Assuming the ultimate step is to upgrade the
>>
> compositor
>
>> to also ES3, this shortcut is short-lived anyway.
>>
>
> What do you think?
>>
>
> It's a good point. As long as these checks are only being done for the
> textures consumed by the active program, I guess it sounds OK to remove
> the short-circuit. We need to be careful to not regress per-draw-call
> performance.
>
> https://codereview.chromium.org/1505343003/
>
--
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.
On 2015/12/15 07:36:42, piman (in KST timezone) wrote:
> Ideally this validation would be done on binds and changes, rather than
> draws ("pay for what you use"). It may require a little bit more tracking.
> 1- for the common case where samplers aren't used (es2, and virtually all
> existing clients), keep track of the status assuming the Texture's sampler
> state is used
> 2- on a per-unit basis (in the context), keep track of the status
> corresponding to the texture/sampler combination. When the Texture is bound
> and the current sampler is 0, use the Texture's validation state. When the
> texture is bound and the current sampler is !0, or when a sampler is bound,
> revalidate using the combination of Texture+Sampler.
> 3- when a Texture is changed, revalidate all units in the current context
> where it is bound (including the active one). There can be some
> optimizations there if needed (e.g. keep track on a per-Texture basis of
> how many units it's bound to, across all contexts. Only revalidate all
> units beyond the active one if the Texture is bound to >1 unit, which will
> be uncommon). Could do it lazily (keep a dirty bit) to only revalidate once
> over a series of TexParameter changes (common at initialization time).
> 4- when a Sampler is changed, same.
> 5- on a context switch, revalidate all units (if needed we could keep a
> version number on the TextureManager to avoid that).
We discussed this approach at the ANGLE team meeting today and don't think this
is the best direction to go. The basic observation is that there are typically a
lot of texture binds whose effects aren't seen, because they're intermediate
states on the way to the next draw call. Doing the validation during binds and
texture mutations will over-validate.
ANGLE defers the texture/sampler validation until the next draw call. For the
(hopefully brief) time that the command buffer is doing the same, I think the
command buffer should follow ANGLE's approach.
>
> On Tue, Dec 15, 2015 at 9:27 AM, <mailto:kbr@chromium.org> wrote:
>
> >
> >
> >
>
https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> >
> >
> >
>
https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:7663: if
> > (!texture_manager()->HaveImages()) {
> > On 2015/12/14 23:58:17, Zhenyao Mo wrote:
> >
> >> On 2015/12/14 23:22:44, Ken Russell wrote:
> >> > On 2015/12/14 22:48:33, Zhenyao Mo wrote:
> >> > > This is wrong. Now we can't short cut here as we don't know
> >>
> > unrenderable
> >
> >> > > texture count.
> >> >
> >> > Could this be updated so that at least ES 2.0 contexts can still
> >>
> > short-cut
> >
> >> these
> >> > checks if there aren't any unrenderable textures in the texture
> >>
> > manager?
> >
> > Now that we could possibly share textures among ES2/ES3 context
> >>
> > through mailbox,
> >
> >> we really can't decide until render time. Unless we have
> >>
> > RENDERABLE_ES2 and
> >
> >> HaveUnrenderableTexturesES2(), but then all the designs to unify
> >>
> > ES2/ES3 texture
> >
> >> handling will be lost. Assuming the ultimate step is to upgrade the
> >>
> > compositor
> >
> >> to also ES3, this shortcut is short-lived anyway.
> >>
> >
> > What do you think?
> >>
> >
> > It's a good point. As long as these checks are only being done for the
> > textures consumed by the active program, I guess it sounds OK to remove
> > the short-circuit. We need to be careful to not regress per-draw-call
> > performance.
> >
> > https://codereview.chromium.org/1505343003/
> >
>
> --
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 ========== to ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 ==========
On Wed, Dec 16, 2015 at 9:30 AM, <kbr@chromium.org> wrote: > On 2015/12/15 07:36:42, piman (in KST timezone) wrote: > >> Ideally this validation would be done on binds and changes, rather than >> draws ("pay for what you use"). It may require a little bit more tracking. >> 1- for the common case where samplers aren't used (es2, and virtually all >> existing clients), keep track of the status assuming the Texture's sampler >> state is used >> 2- on a per-unit basis (in the context), keep track of the status >> corresponding to the texture/sampler combination. When the Texture is >> bound >> and the current sampler is 0, use the Texture's validation state. When the >> texture is bound and the current sampler is !0, or when a sampler is >> bound, >> revalidate using the combination of Texture+Sampler. >> 3- when a Texture is changed, revalidate all units in the current context >> where it is bound (including the active one). There can be some >> optimizations there if needed (e.g. keep track on a per-Texture basis of >> how many units it's bound to, across all contexts. Only revalidate all >> units beyond the active one if the Texture is bound to >1 unit, which will >> be uncommon). Could do it lazily (keep a dirty bit) to only revalidate >> once >> over a series of TexParameter changes (common at initialization time). >> 4- when a Sampler is changed, same. >> 5- on a context switch, revalidate all units (if needed we could keep a >> version number on the TextureManager to avoid that). >> > > We discussed this approach at the ANGLE team meeting today and don't think > this > is the best direction to go. The basic observation is that there are > typically a > lot of texture binds whose effects aren't seen, because they're > intermediate > states on the way to the next draw call. Doing the validation during binds > and > texture mutations will over-validate. > > ANGLE defers the texture/sampler validation until the next draw call. For > the > (hopefully brief) time that the command buffer is doing the same, I think > the > command buffer should follow ANGLE's approach. Whatever we do, we need to ensure that we keep the property that draw calls are as cheap as possible. This affects every single draw call for every single frame on every single page for every single user. If the claim is that this change is negligible, I want to see the numbers. > > > > On Tue, Dec 15, 2015 at 9:27 AM, <mailto:kbr@chromium.org> wrote: >> > > > >> > >> > >> > > > https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... > >> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): >> > >> > >> > >> > > > https://codereview.chromium.org/1505343003/diff/100001/gpu/command_buffer/ser... > >> > gpu/command_buffer/service/gles2_cmd_decoder.cc:7663: if >> > (!texture_manager()->HaveImages()) { >> > On 2015/12/14 23:58:17, Zhenyao Mo wrote: >> > >> >> On 2015/12/14 23:22:44, Ken Russell wrote: >> >> > On 2015/12/14 22:48:33, Zhenyao Mo wrote: >> >> > > This is wrong. Now we can't short cut here as we don't know >> >> >> > unrenderable >> > >> >> > > texture count. >> >> > >> >> > Could this be updated so that at least ES 2.0 contexts can still >> >> >> > short-cut >> > >> >> these >> >> > checks if there aren't any unrenderable textures in the texture >> >> >> > manager? >> > >> > Now that we could possibly share textures among ES2/ES3 context >> >> >> > through mailbox, >> > >> >> we really can't decide until render time. Unless we have >> >> >> > RENDERABLE_ES2 and >> > >> >> HaveUnrenderableTexturesES2(), but then all the designs to unify >> >> >> > ES2/ES3 texture >> > >> >> handling will be lost. Assuming the ultimate step is to upgrade the >> >> >> > compositor >> > >> >> to also ES3, this shortcut is short-lived anyway. >> >> >> > >> > What do you think? >> >> >> > >> > It's a good point. As long as these checks are only being done for the >> > textures consumed by the active program, I guess it sounds OK to remove >> > the short-circuit. We need to be careful to not regress per-draw-call >> > performance. >> > >> > https://codereview.chromium.org/1505343003/ >> > >> > > -- >> 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 mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/1505343003/ > -- 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.
piman@chromium.org changed reviewers: + vmiura@chromium.org
+vmiura who can point at how to profile the command buffer code, especially on Android which is probably the most sensitive.
> +vmiura who can point at how to profile the command buffer code, especially on > Android which is probably the most sensitive. Could you try the Benchmark: thread_times.tough_compositor_cases on a Nexus 4 or 5? Check thread_GPU_cpu_time_per_frame on a page like CC_SCROLL_200_LAYER_GRID (good number of draw calls, low noise). With changes like this coming up, and the need to keep track of any regressions with MANGLE, it would be great to have some targeted micro benchmarks.
Took me a while to get around to doing the benchmark, sorry. Had a lot of trouble getting my Nexus 4 set up for it and finally just borrowed vmiura@'s. :P Running thread_times.tough_compositor_cases on a Nexus 4 and watching the thread_GPU_cpu_time_per_frame I see effectively no difference. Top of tree times: 7.577104ms 7.570829ms 7.512818ms With this CL: 7.465083ms 7.520272ms 7.554637ms
Sorry, should have explained: the three times are from the three separate runs I did to make sure we were getting consistent numbers.
On 2016/01/15 23:26:22, bajones wrote: > Sorry, should have explained: the three times are from the three separate runs I > did to make sure we were getting consistent numbers. Review ping after the long weekend. I'm not seeing that this change adversely affects draw performance.
https://codereview.chromium.org/1505343003/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1505343003/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7597: if (!texture_manager()->HaveImages()) { Hmm, actually, don't you need to go through the code below always? Since you don't know whether or not the texture is complete any more.
Fixed the bad if, please take another look. https://codereview.chromium.org/1505343003/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1505343003/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7597: if (!texture_manager()->HaveImages()) { On 2016/01/20 01:14:02, piman (Slow to review) wrote: > Hmm, actually, don't you need to go through the code below always? Since you > don't know whether or not the texture is complete any more. Good catch, I had read the original code as an || by mistake.
On Wed, Jan 20, 2016 at 3:57 PM, <bajones@chromium.org> wrote: > Fixed the bad if, please take another look. Are the benchmarks still showing no regression? > > > > > https://codereview.chromium.org/1505343003/diff/120001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > https://codereview.chromium.org/1505343003/diff/120001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:7597: if > (!texture_manager()->HaveImages()) { > On 2016/01/20 01:14:02, piman (Slow to review) wrote: > >> Hmm, actually, don't you need to go through the code below always? >> > Since you > >> don't know whether or not the texture is complete any more. >> > > Good catch, I had read the original code as an || by mistake. > > https://codereview.chromium.org/1505343003/ > -- 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.
On 2016/01/21 00:37:57, piman (Slow to review) wrote: > Are the benchmarks still showing no regression? With this CL: Avg thread_GPU_cpu_time_per_frame: 7.612486ms Sd thread_GPU_cpu_time_per_frame: 7.486029ms Avg thread_GPU_cpu_time_per_frame: 7.798101ms Sd thread_GPU_cpu_time_per_frame: 7.923276ms Avg thread_GPU_cpu_time_per_frame: 7.810679ms Sd thread_GPU_cpu_time_per_frame: 7.874330ms Without this CL: Avg thread_GPU_cpu_time_per_frame: 7.589374ms Sd thread_GPU_cpu_time_per_frame: 7.543208ms Avg thread_GPU_cpu_time_per_frame: 7.723999ms Sd thread_GPU_cpu_time_per_frame: 7.548653ms Avg thread_GPU_cpu_time_per_frame: 7.833390ms Sd thread_GPU_cpu_time_per_frame: 7.899717ms The averages are ever so slightly higher (~0.02ms) but that seems like it could easily just be noise.
LGTM, let's land this, but please watch the perf bots for regressions.
On 2016/01/21 23:04:55, piman (Slow to review) wrote: > LGTM, let's land this, but please watch the perf bots for regressions. Will do!
The CQ bit was checked by bajones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1505343003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1505343003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by bajones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/1505343003/#ps160001 (title: "Fixed unittest that was checking the wrong value")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1505343003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1505343003/160001
Message was sent while issue was closed.
Description was changed from ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 ========== to ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 ========== to ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 Committed: https://crrev.com/838ca1836eae1357a3b9135e808266d7d28bd3fa Cr-Commit-Position: refs/heads/master@{#371062} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/838ca1836eae1357a3b9135e808266d7d28bd3fa Cr-Commit-Position: refs/heads/master@{#371062}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/1630263002/ by tapted@chromium.org. The reason for reverting is: Suspected for failures on Android/Nexus in WebglConformance.conformance_extensions_oes_texture_float Starting in https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus... Failing tests: [ FAILED ] 3 tests, listed below: [ FAILED ] WebglConformance.conformance_extensions_oes_texture_float_with_image_data [ FAILED ] WebglConformance.conformance_extensions_oes_texture_float_with_image [ FAILED ] WebglConformance.conformance_extensions_oes_texture_float See BUG=580908.
Message was sent while issue was closed.
Description was changed from ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 Committed: https://crrev.com/838ca1836eae1357a3b9135e808266d7d28bd3fa Cr-Commit-Position: refs/heads/master@{#371062} ========== to ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 Committed: https://crrev.com/838ca1836eae1357a3b9135e808266d7d28bd3fa Cr-Commit-Position: refs/heads/master@{#371062} ==========
Figured out the Android failures. There was a subtle change to the validation logic for GL_FLOAT and GL_HALF_FLOAT textures that was causing them to fail if linear filtering of floating point textures wasn't supported, even if the texture wasn't using a linear filtered mode. That's been corrected in patchset 10, so I'm going to re-land.
The CQ bit was checked by bajones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/1505343003/#ps180001 (title: "Fixed issue on devices without TEXTURE_FLOAT_LINEAR")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1505343003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1505343003/180001
Message was sent while issue was closed.
Description was changed from ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 Committed: https://crrev.com/838ca1836eae1357a3b9135e808266d7d28bd3fa Cr-Commit-Position: refs/heads/master@{#371062} ========== to ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 Committed: https://crrev.com/838ca1836eae1357a3b9135e808266d7d28bd3fa Cr-Commit-Position: refs/heads/master@{#371062} ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 Committed: https://crrev.com/838ca1836eae1357a3b9135e808266d7d28bd3fa Cr-Commit-Position: refs/heads/master@{#371062} ========== to ========== Updating texture validation to account for sampler objects in ES3 BUG=563613 Committed: https://crrev.com/838ca1836eae1357a3b9135e808266d7d28bd3fa Cr-Commit-Position: refs/heads/master@{#371062} Committed: https://crrev.com/7fd547659babda857da554a4d4514df0779aeb74 Cr-Commit-Position: refs/heads/master@{#372187} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/7fd547659babda857da554a4d4514df0779aeb74 Cr-Commit-Position: refs/heads/master@{#372187} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
