Re: [Mesa-dev] [PATCH 05/29] mesa: Consider internal base format in _mesa_format_convert

2014-11-20 Thread Iago Toral 
On Thu, 2014-11-20 at 08:42 +0100, Iago Toral wrote:
 On Wed, 2014-11-19 at 11:43 -0800, Jason Ekstrand wrote:
  A couple of specific comments are below.  More generally, why are you
  only considering the base format on two cases?  Do we never use it for
  anything else?
 
 I thought about that too but when I looked at the original code it
 seemed that it only cared for the base format in these two scenarios, so
 I thought that maybe the conversions cases that could be affected are
 all handled in those two paths. I'll check again though, just in case I
 missed something.

I don't know how I came to that conclusion but it seems wrong after
looking at the original code in texstore.c, which considers the base
internal format in the integer, float and ubyte paths too, so we should
do the same in _mesa_format_convert. I'll fix that.

Iago

  On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga
  ito...@igalia.com wrote:
  Add a dst_internal_format parameter to _mesa_format_convert,
  that represents
  the base internal format for texture/pixel uploads, so we can
  do the right
  thing when the driver has selected a different internal format
  for the target
  dst format.
  ---
   src/mesa/main/format_utils.c | 65
  +++-
   src/mesa/main/format_utils.h |  2 +-
   2 files changed, 65 insertions(+), 2 deletions(-)
  
  diff --git a/src/mesa/main/format_utils.c
  b/src/mesa/main/format_utils.c
  index fc59e86..5964689 100644
  --- a/src/mesa/main/format_utils.c
  +++ b/src/mesa/main/format_utils.c
  @@ -303,7 +303,7 @@ _mesa_compute_component_mapping(GLenum
  inFormat, GLenum outFormat, GLubyte *map)
   void
   _mesa_format_convert(void *void_dst, uint32_t dst_format,
  size_t dst_stride,
void *void_src, uint32_t src_format,
  size_t src_stride,
  - size_t width, size_t height)
  + size_t width, size_t height, GLenum
  dst_internal_format)
   {
  uint8_t *dst = (uint8_t *)void_dst;
  uint8_t *src = (uint8_t *)void_src;
  @@ -422,6 +422,36 @@ _mesa_format_convert(void *void_dst,
  uint32_t dst_format, size_t dst_stride,
  if (src_array_format.as_uint  dst_array_format.as_uint)
  {
 assert(src_array_format.normalized ==
  dst_array_format.normalized);
  
  +  /* If the base format of our dst is not the same as the
  provided base
  +   * format it means that we are converting to a
  different format
  +   * than the one originally requested by the client.
  This can happen when
  +   * the internal base format requested is not supported
  by the driver.
  +   * In this case we need to consider the requested
  internal base format to
  +   * compute the correct swizzle operation from src to
  dst. We will do this
  +   * by computing the swizzle transform
  src-rgba-base-rgba-dst instead
  +   * of src-rgba-dst.
  +   */
  +  mesa_format dst_mesa_format;
  +  if (dst_format  MESA_ARRAY_FORMAT_BIT)
  + dst_mesa_format =
  _mesa_format_from_array_format(dst_format);
  +  else
  + dst_mesa_format = dst_format;
  
  
  Let's put an extra line here so it doesn't get confused with the below
  if statement
  
   
  +  if (dst_internal_format !=
  _mesa_get_format_base_format(dst_mesa_format)) {
  + /* Compute src2rgba as src-rgba-base-rgba */
  + uint8_t rgba2base[4], base2rgba[4], swizzle[4];
  + _mesa_compute_component_mapping(GL_RGBA,
  dst_internal_format, rgba2base);
  + _mesa_compute_component_mapping(dst_internal_format,
  GL_RGBA, base2rgba);
  +
  + for (i = 0; i  4; i++) {
  +if (base2rgba[i]  MESA_FORMAT_SWIZZLE_W)
  +   swizzle[i] = base2rgba[i];
  +else
  +   swizzle[i] =
  src2rgba[rgba2base[base2rgba[i]]];
  
  
  This doesn't work for composing three swizzles.  If you get a ZERO or
  ONE in rgba2base, you'll read the wrong memory from src2rgba.
 
 Actually, the problem is worse, because the mapping written by
 _mesa_compute_component_mapping is a 6-component mapping and we are
 passing a 4-component array. I'll fix this.
 
   
  
  + }
  + memcpy(src2rgba, swizzle, sizeof(src2rgba));
  +  }
  +
  +  /* Compute src2dst from src2rgba */
 for (i = 0; i  4; i++) {

Re: [Mesa-dev] [PATCH 05/29] mesa: Consider internal base format in _mesa_format_convert

2014-11-20 Thread Jason Ekstrand 
On Wed, Nov 19, 2014 at 11:42 PM, Iago Toral ito...@igalia.com wrote:

 On Wed, 2014-11-19 at 11:43 -0800, Jason Ekstrand wrote:
  A couple of specific comments are below.  More generally, why are you
  only considering the base format on two cases?  Do we never use it for
  anything else?

 I thought about that too but when I looked at the original code it
 seemed that it only cared for the base format in these two scenarios, so
 I thought that maybe the conversions cases that could be affected are
 all handled in those two paths. I'll check again though, just in case I
 missed something.

  On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga
  ito...@igalia.com wrote:
  Add a dst_internal_format parameter to _mesa_format_convert,
  that represents
  the base internal format for texture/pixel uploads, so we can
  do the right
  thing when the driver has selected a different internal format
  for the target
  dst format.
  ---
   src/mesa/main/format_utils.c | 65
  +++-
   src/mesa/main/format_utils.h |  2 +-
   2 files changed, 65 insertions(+), 2 deletions(-)
 
  diff --git a/src/mesa/main/format_utils.c
  b/src/mesa/main/format_utils.c
  index fc59e86..5964689 100644
  --- a/src/mesa/main/format_utils.c
  +++ b/src/mesa/main/format_utils.c
  @@ -303,7 +303,7 @@ _mesa_compute_component_mapping(GLenum
  inFormat, GLenum outFormat, GLubyte *map)
   void
   _mesa_format_convert(void *void_dst, uint32_t dst_format,
  size_t dst_stride,
void *void_src, uint32_t src_format,
  size_t src_stride,
  - size_t width, size_t height)
  + size_t width, size_t height, GLenum
  dst_internal_format)
   {
  uint8_t *dst = (uint8_t *)void_dst;
  uint8_t *src = (uint8_t *)void_src;
  @@ -422,6 +422,36 @@ _mesa_format_convert(void *void_dst,
  uint32_t dst_format, size_t dst_stride,
  if (src_array_format.as_uint  dst_array_format.as_uint)
  {
 assert(src_array_format.normalized ==
  dst_array_format.normalized);
 
  +  /* If the base format of our dst is not the same as the
  provided base
  +   * format it means that we are converting to a
  different format
  +   * than the one originally requested by the client.
  This can happen when
  +   * the internal base format requested is not supported
  by the driver.
  +   * In this case we need to consider the requested
  internal base format to
  +   * compute the correct swizzle operation from src to
  dst. We will do this
  +   * by computing the swizzle transform
  src-rgba-base-rgba-dst instead
  +   * of src-rgba-dst.
  +   */
  +  mesa_format dst_mesa_format;
  +  if (dst_format  MESA_ARRAY_FORMAT_BIT)
  + dst_mesa_format =
  _mesa_format_from_array_format(dst_format);
  +  else
  + dst_mesa_format = dst_format;
 
 
  Let's put an extra line here so it doesn't get confused with the below
  if statement
 
 
  +  if (dst_internal_format !=
  _mesa_get_format_base_format(dst_mesa_format)) {
  + /* Compute src2rgba as src-rgba-base-rgba */
  + uint8_t rgba2base[4], base2rgba[4], swizzle[4];
  + _mesa_compute_component_mapping(GL_RGBA,
  dst_internal_format, rgba2base);
  + _mesa_compute_component_mapping(dst_internal_format,
  GL_RGBA, base2rgba);
  +
  + for (i = 0; i  4; i++) {
  +if (base2rgba[i]  MESA_FORMAT_SWIZZLE_W)
  +   swizzle[i] = base2rgba[i];
  +else
  +   swizzle[i] =
  src2rgba[rgba2base[base2rgba[i]]];
 
 
  This doesn't work for composing three swizzles.  If you get a ZERO or
  ONE in rgba2base, you'll read the wrong memory from src2rgba.

 Actually, the problem is worse, because the mapping written by
 _mesa_compute_component_mapping is a 6-component mapping and we are
 passing a 4-component array. I'll fix this.


Right now we have 2 different swizzle formats floating around.  Some with 6
components and some with 4.  4 works just fine as long as you are careful
when you compose swizzles.  With 6 you can avoid the if statements because
4 and 5 simply pass through.  I like 4 better (which is why I did it that
way in my stuff) but either works.



 
 
  + }
  + memcpy(src2rgba, swizzle, sizeof(src2rgba));
  +  }
  +
  +  /* Compute src2dst from

Re: [Mesa-dev] [PATCH 05/29] mesa: Consider internal base format in _mesa_format_convert

2014-11-19 Thread Jason Ekstrand 
A couple of specific comments are below.  More generally, why are you only
considering the base format on two cases?  Do we never use it for anything
else?

On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga ito...@igalia.com
wrote:

 Add a dst_internal_format parameter to _mesa_format_convert, that
 represents
 the base internal format for texture/pixel uploads, so we can do the right
 thing when the driver has selected a different internal format for the
 target
 dst format.
 ---
  src/mesa/main/format_utils.c | 65
 +++-
  src/mesa/main/format_utils.h |  2 +-
  2 files changed, 65 insertions(+), 2 deletions(-)

 diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c
 index fc59e86..5964689 100644
 --- a/src/mesa/main/format_utils.c
 +++ b/src/mesa/main/format_utils.c
 @@ -303,7 +303,7 @@ _mesa_compute_component_mapping(GLenum inFormat,
 GLenum outFormat, GLubyte *map)
  void
  _mesa_format_convert(void *void_dst, uint32_t dst_format, size_t
 dst_stride,
   void *void_src, uint32_t src_format, size_t
 src_stride,
 - size_t width, size_t height)
 + size_t width, size_t height, GLenum
 dst_internal_format)
  {
 uint8_t *dst = (uint8_t *)void_dst;
 uint8_t *src = (uint8_t *)void_src;
 @@ -422,6 +422,36 @@ _mesa_format_convert(void *void_dst, uint32_t
 dst_format, size_t dst_stride,
 if (src_array_format.as_uint  dst_array_format.as_uint) {
assert(src_array_format.normalized == dst_array_format.normalized);

 +  /* If the base format of our dst is not the same as the provided
 base
 +   * format it means that we are converting to a different format
 +   * than the one originally requested by the client. This can happen
 when
 +   * the internal base format requested is not supported by the
 driver.
 +   * In this case we need to consider the requested internal base
 format to
 +   * compute the correct swizzle operation from src to dst. We will
 do this
 +   * by computing the swizzle transform src-rgba-base-rgba-dst
 instead
 +   * of src-rgba-dst.
 +   */
 +  mesa_format dst_mesa_format;
 +  if (dst_format  MESA_ARRAY_FORMAT_BIT)
 + dst_mesa_format = _mesa_format_from_array_format(dst_format);
 +  else
 + dst_mesa_format = dst_format;


Let's put an extra line here so it doesn't get confused with the below if
statement


 +  if (dst_internal_format !=
 _mesa_get_format_base_format(dst_mesa_format)) {
 + /* Compute src2rgba as src-rgba-base-rgba */
 + uint8_t rgba2base[4], base2rgba[4], swizzle[4];
 + _mesa_compute_component_mapping(GL_RGBA, dst_internal_format,
 rgba2base);
 + _mesa_compute_component_mapping(dst_internal_format, GL_RGBA,
 base2rgba);
 +
 + for (i = 0; i  4; i++) {
 +if (base2rgba[i]  MESA_FORMAT_SWIZZLE_W)
 +   swizzle[i] = base2rgba[i];
 +else
 +   swizzle[i] = src2rgba[rgba2base[base2rgba[i]]];


This doesn't work for composing three swizzles.  If you get a ZERO or ONE
in rgba2base, you'll read the wrong memory from src2rgba.


 + }
 + memcpy(src2rgba, swizzle, sizeof(src2rgba));
 +  }
 +
 +  /* Compute src2dst from src2rgba */
for (i = 0; i  4; i++) {
   if (rgba2dst[i]  MESA_FORMAT_SWIZZLE_W) {
  src2dst[i] = rgba2dst[i];
 @@ -539,9 +569,42 @@ _mesa_format_convert(void *void_dst, uint32_t
 dst_format, size_t dst_stride,
  src += src_stride;
   }
} else {
 + /* For some conversions, doing src-rgba-dst is not enough and
 we
 +  * need to consider the base internal format. In these cases a
 +  * swizzle operation is required to match the semantics of the
 base
 +  * internal format requested: src-rgba-swizzle-rgba-dst.
 +  *
 +  * We can detect these cases by checking if the swizzle transform
 +  * for base-rgba-base is 0123. If it is not, then we need
 +  * to do the swizzle operation (need_convert = true).
 +  */
 + GLubyte rgba2base[4], base2rgba[4], map[4];
 + bool need_convert = false;
 + mesa_format dst_mesa_format;
 + if (dst_format  MESA_ARRAY_FORMAT_BIT)
 +dst_mesa_format = _mesa_format_from_array_format(dst_format);
 + else
 +dst_mesa_format = dst_format;


Blank line again

+ if (dst_internal_format !=
 + _mesa_get_format_base_format(dst_mesa_format)) {
 +_mesa_compute_component_mapping(GL_RGBA, dst_internal_format,
 +base2rgba);
 +_mesa_compute_component_mapping(dst_internal_format, GL_RGBA,
 +rgba2base);
 +for (i = 0; i  4; ++i) {
 +   map[i] = base2rgba[rgba2base[i]];
 +   if (map[i] != i)
 +

Re: [Mesa-dev] [PATCH 05/29] mesa: Consider internal base format in _mesa_format_convert

2014-11-19 Thread Iago Toral 
On Wed, 2014-11-19 at 11:43 -0800, Jason Ekstrand wrote:
 A couple of specific comments are below.  More generally, why are you
 only considering the base format on two cases?  Do we never use it for
 anything else?

I thought about that too but when I looked at the original code it
seemed that it only cared for the base format in these two scenarios, so
I thought that maybe the conversions cases that could be affected are
all handled in those two paths. I'll check again though, just in case I
missed something.

 On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga
 ito...@igalia.com wrote:
 Add a dst_internal_format parameter to _mesa_format_convert,
 that represents
 the base internal format for texture/pixel uploads, so we can
 do the right
 thing when the driver has selected a different internal format
 for the target
 dst format.
 ---
  src/mesa/main/format_utils.c | 65
 +++-
  src/mesa/main/format_utils.h |  2 +-
  2 files changed, 65 insertions(+), 2 deletions(-)
 
 diff --git a/src/mesa/main/format_utils.c
 b/src/mesa/main/format_utils.c
 index fc59e86..5964689 100644
 --- a/src/mesa/main/format_utils.c
 +++ b/src/mesa/main/format_utils.c
 @@ -303,7 +303,7 @@ _mesa_compute_component_mapping(GLenum
 inFormat, GLenum outFormat, GLubyte *map)
  void
  _mesa_format_convert(void *void_dst, uint32_t dst_format,
 size_t dst_stride,
   void *void_src, uint32_t src_format,
 size_t src_stride,
 - size_t width, size_t height)
 + size_t width, size_t height, GLenum
 dst_internal_format)
  {
 uint8_t *dst = (uint8_t *)void_dst;
 uint8_t *src = (uint8_t *)void_src;
 @@ -422,6 +422,36 @@ _mesa_format_convert(void *void_dst,
 uint32_t dst_format, size_t dst_stride,
 if (src_array_format.as_uint  dst_array_format.as_uint)
 {
assert(src_array_format.normalized ==
 dst_array_format.normalized);
 
 +  /* If the base format of our dst is not the same as the
 provided base
 +   * format it means that we are converting to a
 different format
 +   * than the one originally requested by the client.
 This can happen when
 +   * the internal base format requested is not supported
 by the driver.
 +   * In this case we need to consider the requested
 internal base format to
 +   * compute the correct swizzle operation from src to
 dst. We will do this
 +   * by computing the swizzle transform
 src-rgba-base-rgba-dst instead
 +   * of src-rgba-dst.
 +   */
 +  mesa_format dst_mesa_format;
 +  if (dst_format  MESA_ARRAY_FORMAT_BIT)
 + dst_mesa_format =
 _mesa_format_from_array_format(dst_format);
 +  else
 + dst_mesa_format = dst_format;
 
 
 Let's put an extra line here so it doesn't get confused with the below
 if statement
 
  
 +  if (dst_internal_format !=
 _mesa_get_format_base_format(dst_mesa_format)) {
 + /* Compute src2rgba as src-rgba-base-rgba */
 + uint8_t rgba2base[4], base2rgba[4], swizzle[4];
 + _mesa_compute_component_mapping(GL_RGBA,
 dst_internal_format, rgba2base);
 + _mesa_compute_component_mapping(dst_internal_format,
 GL_RGBA, base2rgba);
 +
 + for (i = 0; i  4; i++) {
 +if (base2rgba[i]  MESA_FORMAT_SWIZZLE_W)
 +   swizzle[i] = base2rgba[i];
 +else
 +   swizzle[i] =
 src2rgba[rgba2base[base2rgba[i]]];
 
 
 This doesn't work for composing three swizzles.  If you get a ZERO or
 ONE in rgba2base, you'll read the wrong memory from src2rgba.

Actually, the problem is worse, because the mapping written by
_mesa_compute_component_mapping is a 6-component mapping and we are
passing a 4-component array. I'll fix this.

  
 
 + }
 + memcpy(src2rgba, swizzle, sizeof(src2rgba));
 +  }
 +
 +  /* Compute src2dst from src2rgba */
for (i = 0; i  4; i++) {
   if (rgba2dst[i]  MESA_FORMAT_SWIZZLE_W) {
  src2dst[i] = rgba2dst[i];
 @@ -539,9 +569,42 @@ _mesa_format_convert(void *void_dst,
 uint32_t dst_format, size_t dst_stride,
  src += src_stride;
   }
} else {
 + /* For some conversions, doing src-rgba-dst is not
 enough and we
 +  * need to

[Mesa-dev] [PATCH 05/29] mesa: Consider internal base format in _mesa_format_convert

2014-11-18 Thread Iago Toral Quiroga 
Add a dst_internal_format parameter to _mesa_format_convert, that represents
the base internal format for texture/pixel uploads, so we can do the right
thing when the driver has selected a different internal format for the target
dst format.
---
 src/mesa/main/format_utils.c | 65 +++-
 src/mesa/main/format_utils.h |  2 +-
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c
index fc59e86..5964689 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -303,7 +303,7 @@ _mesa_compute_component_mapping(GLenum inFormat, GLenum 
outFormat, GLubyte *map)
 void
 _mesa_format_convert(void *void_dst, uint32_t dst_format, size_t dst_stride,
  void *void_src, uint32_t src_format, size_t src_stride,
- size_t width, size_t height)
+ size_t width, size_t height, GLenum dst_internal_format)
 {
uint8_t *dst = (uint8_t *)void_dst;
uint8_t *src = (uint8_t *)void_src;
@@ -422,6 +422,36 @@ _mesa_format_convert(void *void_dst, uint32_t dst_format, 
size_t dst_stride,
if (src_array_format.as_uint  dst_array_format.as_uint) {
   assert(src_array_format.normalized == dst_array_format.normalized);
 
+  /* If the base format of our dst is not the same as the provided base
+   * format it means that we are converting to a different format
+   * than the one originally requested by the client. This can happen when
+   * the internal base format requested is not supported by the driver.
+   * In this case we need to consider the requested internal base format to
+   * compute the correct swizzle operation from src to dst. We will do this
+   * by computing the swizzle transform src-rgba-base-rgba-dst instead
+   * of src-rgba-dst.
+   */
+  mesa_format dst_mesa_format;
+  if (dst_format  MESA_ARRAY_FORMAT_BIT)
+ dst_mesa_format = _mesa_format_from_array_format(dst_format);
+  else
+ dst_mesa_format = dst_format;
+  if (dst_internal_format != 
_mesa_get_format_base_format(dst_mesa_format)) {
+ /* Compute src2rgba as src-rgba-base-rgba */
+ uint8_t rgba2base[4], base2rgba[4], swizzle[4];
+ _mesa_compute_component_mapping(GL_RGBA, dst_internal_format, 
rgba2base);
+ _mesa_compute_component_mapping(dst_internal_format, GL_RGBA, 
base2rgba);
+
+ for (i = 0; i  4; i++) {
+if (base2rgba[i]  MESA_FORMAT_SWIZZLE_W)
+   swizzle[i] = base2rgba[i];
+else
+   swizzle[i] = src2rgba[rgba2base[base2rgba[i]]];
+ }
+ memcpy(src2rgba, swizzle, sizeof(src2rgba));
+  }
+
+  /* Compute src2dst from src2rgba */
   for (i = 0; i  4; i++) {
  if (rgba2dst[i]  MESA_FORMAT_SWIZZLE_W) {
 src2dst[i] = rgba2dst[i];
@@ -539,9 +569,42 @@ _mesa_format_convert(void *void_dst, uint32_t dst_format, 
size_t dst_stride,
 src += src_stride;
  }
   } else {
+ /* For some conversions, doing src-rgba-dst is not enough and we
+  * need to consider the base internal format. In these cases a
+  * swizzle operation is required to match the semantics of the base
+  * internal format requested: src-rgba-swizzle-rgba-dst.
+  *
+  * We can detect these cases by checking if the swizzle transform
+  * for base-rgba-base is 0123. If it is not, then we need
+  * to do the swizzle operation (need_convert = true).
+  */
+ GLubyte rgba2base[4], base2rgba[4], map[4];
+ bool need_convert = false;
+ mesa_format dst_mesa_format;
+ if (dst_format  MESA_ARRAY_FORMAT_BIT)
+dst_mesa_format = _mesa_format_from_array_format(dst_format);
+ else
+dst_mesa_format = dst_format;
+ if (dst_internal_format !=
+ _mesa_get_format_base_format(dst_mesa_format)) {
+_mesa_compute_component_mapping(GL_RGBA, dst_internal_format,
+base2rgba);
+_mesa_compute_component_mapping(dst_internal_format, GL_RGBA,
+rgba2base);
+for (i = 0; i  4; ++i) {
+   map[i] = base2rgba[rgba2base[i]];
+   if (map[i] != i)
+  need_convert = true;
+}
+ }
+
  for (row = 0; row  height; ++row) {
 _mesa_unpack_rgba_row(src_format, width,
   src, tmp_float + row * width);
+if (need_convert)
+   _mesa_swizzle_and_convert(tmp_float + row * width, GL_FLOAT, 4,
+ tmp_float + row * width, GL_FLOAT, 4,
+ map, false, width);
 src += src_stride;
  }
   }
diff --git a/src/mesa/main/format_utils.h