Type conversion fixes, gpu/ edition.

gpu/command_buffer/service/gles2_cmd_decoder.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "gpu/command_buffer/service/gles2_cmd_decoder.h"
 
 #include <stdio.h>
 
 #include <algorithm>
 #include <list>
@@ skipping 3113 matching lines @@
 }
 
 bool GLES2DecoderImpl::CheckFramebufferValid(
     Framebuffer* framebuffer,
     GLenum target, const char* func_name) {
   if (!framebuffer) {
     if (surfaceless_)
       return false;
     if (backbuffer_needs_clear_bits_) {
       glClearColor(0, 0, 0, (GLES2Util::GetChannelsForFormat(
-          offscreen_target_color_format_) & 0x0008) != 0 ? 0 : 1);
+          offscreen_target_color_format_) & 0x0008) != 0 ? 0 : 1.f);
       state_.SetDeviceColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
       glClearStencil(0);
       state_.SetDeviceStencilMaskSeparate(GL_FRONT, kDefaultStencilMask);
       state_.SetDeviceStencilMaskSeparate(GL_BACK, kDefaultStencilMask);
       glClearDepth(1.0f);
       state_.SetDeviceDepthMask(GL_TRUE);
       state_.SetDeviceCapabilityState(GL_SCISSOR_TEST, false);
       bool reset_draw_buffer = false;
       if ((backbuffer_needs_clear_bits_ | GL_COLOR_BUFFER_BIT) != 0 &&
           group_->draw_buffer() == GL_NONE) {
@@ skipping 543 matching lines @@
       GL_FRAMEBUFFER_COMPLETE) {
       LOG(ERROR) << "GLES2DecoderImpl::ResizeOffscreenFrameBuffer failed "
                  << "because offscreen FBO was incomplete.";
     return false;
   }
 
   // Clear the target frame buffer.
   {
     ScopedFrameBufferBinder binder(this, offscreen_target_frame_buffer_->id());
     glClearColor(0, 0, 0, (GLES2Util::GetChannelsForFormat(
-        offscreen_target_color_format_) & 0x0008) != 0 ? 0 : 1);
+        offscreen_target_color_format_) & 0x0008) != 0 ? 0 : 1.f);
     state_.SetDeviceColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
     glClearStencil(0);
     state_.SetDeviceStencilMaskSeparate(GL_FRONT, kDefaultStencilMask);
     state_.SetDeviceStencilMaskSeparate(GL_BACK, kDefaultStencilMask);
     glClearDepth(0);
     state_.SetDeviceDepthMask(GL_TRUE);
     state_.SetDeviceCapabilityState(GL_SCISSOR_TEST, false);
     glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT);
     RestoreClearState();
   }
@@ skipping 3499 matching lines @@
       LOCAL_SET_GL_ERROR(
           GL_INVALID_VALUE,
           "glVertexAttribPointer", "client side arrays are not allowed");
       return error::kNoError;
     }
   }
 
   GLuint indx = c.indx;
   GLint size = c.size;
   GLenum type = c.type;
-  GLboolean normalized = c.normalized;
+  GLboolean normalized = !!c.normalized;

[Peter Kasting, 2014/10/16 23:20:11]

This is a bit subtle.

c.normalized is a 32-bit value, while GLboolean is an 8-bit type (not a bool).

Therefore, an explicit or implicit cast directly to GLboolean is functionally
equivalent to something like:

  normalized = c.normalized & 0xff;

...meaning that, depending on the value of c.normalized, this could convert a
nonzero source value into a zero destination, or convert something into a
destination value other than 0 or 1.

I assumed this wasn't the intent of the code, and the real intent was more like:

  normalized = c.normalized != 0;

"!!" is simply a shorter way of writing that same expression.  (2 places)

[no sievers, 2014/10/16 23:52:08]

Yes this looks good since this gets set from gles2_cmd_helper_autogen.h where it
expands GLBoolean to 32bits. It's just that in shared memory we want every field
to be 32bit so we can use pragma pack(4).

[Peter Kasting, 2014/10/17 00:00:40]

I see.

I wonder if it might be better to use static_cast<GLboolean>, then, if the
original source value was a GLboolean.  That way, if for some insane reason
someone had a use for an original value > 1, it would be preserved correctly
here.

Which do you think is better?

[no sievers, 2014/10/17 00:06:24]

Yea maybe it's more correct to do the inverse (i.e. static_cast<GLBoolean>) of
the expansion we do in the client (implicit conversion to 32bit) here.

Either way it would be a client bug if that affects behavior :)

   GLsizei stride = c.stride;
   GLsizei offset = c.offset;
   const void* ptr = reinterpret_cast<const void*>(offset);
   if (!validators_->vertex_attrib_type.IsValid(type)) {
     LOCAL_SET_GL_ERROR_INVALID_ENUM("glVertexAttribPointer", type, "type");
     return error::kNoError;
   }
   if (!validators_->vertex_attrib_size.IsValid(size)) {
     LOCAL_SET_GL_ERROR(
         GL_INVALID_VALUE, "glVertexAttribPointer", "size GL_INVALID_VALUE");
@@ skipping 221 matching lines @@
   TRACE_EVENT0("gpu", "GLES2DecoderImpl::HandleReadPixels");
   error::Error fbo_error = WillAccessBoundFramebufferForRead();
   if (fbo_error != error::kNoError)
     return fbo_error;
   GLint x = c.x;
   GLint y = c.y;
   GLsizei width = c.width;
   GLsizei height = c.height;
   GLenum format = c.format;
   GLenum type = c.type;
-  GLboolean async = c.async;
+  GLboolean async = !!c.async;

[no sievers, 2014/10/16 23:52:08]

same here

   if (width < 0 || height < 0) {
     LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, "glReadPixels", "dimensions < 0");
     return error::kNoError;
   }
   typedef cmds::ReadPixels::Result Result;
   uint32 pixels_size;
   if (!GLES2Util::ComputeImageDataSizes(
       width, height, format, type, state_.pack_alignment, &pixels_size,
       NULL, NULL)) {
     return error::kOutOfBounds;
@@ skipping 3718 matching lines @@
   }
 }
 
 // Include the auto-generated part of this file. We split this because it means
 // we can easily edit the non-auto generated parts right here in this file
 // instead of having to edit some template or the code generator.
 #include "gpu/command_buffer/service/gles2_cmd_decoder_autogen.h"
 
 }  // namespace gles2
 }  // namespace gpu