[Mesa-dev] [PATCH] mesa: Add a helper function for shared code in get_tex_rgba_{un}compressed

[Anuj Phogat]

On Wed, Jun 8, 2016 at 3:34 PM, Brian Paul <brianp at vmware.com> wrote:
> On 06/08/2016 12:15 PM, Anuj Phogat wrote:
>>
>> On Mon, Dec 28, 2015 at 10:46 AM, Anuj Phogat <anuj.phogat at gmail.com>
>> wrote:
>>>
>>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>>> ---
>>>   src/mesa/main/texgetimage.c | 83
>>> +++++++++++++++++++++------------------------
>>>   1 file changed, 38 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/src/mesa/main/texgetimage.c b/src/mesa/main/texgetimage.c
>>> index b273aaa..4399803 100644
>>> --- a/src/mesa/main/texgetimage.c
>>> +++ b/src/mesa/main/texgetimage.c
>>> @@ -265,6 +265,40 @@ get_tex_ycbcr(struct gl_context *ctx, GLuint
>>> dimensions,
>>>      }
>>>   }
>>>
>>> +/* Depending on the base format involved we may need to apply a rebase
>>> + * transform (for example: if we download to a Luminance format we want
>>> + * G=0 and B=0).
>>> + */
>>> +
>
>
> How about a doxygen-style comment:
>
> /**
>  * Depending...
>  */
>
> And remove the empty line between the comment and the function.
>
>>> +static bool
>>> +teximage_needs_rebase(mesa_format texFormat, GLenum baseFormat,
>>> +                      bool is_compressed, uint8_t *rebaseSwizzle)
>>> +{
>>> +   bool needsRebase = false;
>>> +
>>> +   if (baseFormat == GL_LUMINANCE ||
>>> +       baseFormat == GL_INTENSITY) {
>>> +      needsRebase = true;
>>> +      rebaseSwizzle[0] = MESA_FORMAT_SWIZZLE_X;
>>> +      rebaseSwizzle[1] = MESA_FORMAT_SWIZZLE_ZERO;
>>> +      rebaseSwizzle[2] = MESA_FORMAT_SWIZZLE_ZERO;
>>> +      rebaseSwizzle[3] = MESA_FORMAT_SWIZZLE_ONE;
>>> +   } else if (baseFormat == GL_LUMINANCE_ALPHA) {
>>> +      needsRebase = true;
>>> +      rebaseSwizzle[0] = MESA_FORMAT_SWIZZLE_X;
>>> +      rebaseSwizzle[1] = MESA_FORMAT_SWIZZLE_ZERO;
>>> +      rebaseSwizzle[2] = MESA_FORMAT_SWIZZLE_ZERO;
>>> +      rebaseSwizzle[3] = MESA_FORMAT_SWIZZLE_W;
>>> +   } else if (!is_compressed && (baseFormat !=
>>> +               _mesa_get_format_base_format(texFormat))) {
>
>
> I think I'd break the line after the && instead of across the !=
>
>
>
>>> +      needsRebase =
>>> +         _mesa_compute_rgba2base2rgba_component_mapping(baseFormat,
>>> +                                                        rebaseSwizzle);
>>> +   }
>>> +
>>> +   return needsRebase;
>>> +}
>>> +
>>>
>>>   /**
>>>    * Get a color texture image with decompression.
>>> @@ -319,26 +353,8 @@ get_tex_rgba_compressed(struct gl_context *ctx,
>>> GLuint dimensions,
>>>         }
>>>      }
>>>
>>> -   /* Depending on the base format involved we may need to apply a
>>> rebase
>>> -    * transform (for example: if we download to a Luminance format we
>>> want
>>> -    * G=0 and B=0).
>>> -    */
>>> -   if (baseFormat == GL_LUMINANCE ||
>>> -       baseFormat == GL_INTENSITY) {
>>> -      needsRebase = true;
>>> -      rebaseSwizzle[0] = MESA_FORMAT_SWIZZLE_X;
>>> -      rebaseSwizzle[1] = MESA_FORMAT_SWIZZLE_ZERO;
>>> -      rebaseSwizzle[2] = MESA_FORMAT_SWIZZLE_ZERO;
>>> -      rebaseSwizzle[3] = MESA_FORMAT_SWIZZLE_ONE;
>>> -   } else if (baseFormat == GL_LUMINANCE_ALPHA) {
>>> -      needsRebase = true;
>>> -      rebaseSwizzle[0] = MESA_FORMAT_SWIZZLE_X;
>>> -      rebaseSwizzle[1] = MESA_FORMAT_SWIZZLE_ZERO;
>>> -      rebaseSwizzle[2] = MESA_FORMAT_SWIZZLE_ZERO;
>>> -      rebaseSwizzle[3] = MESA_FORMAT_SWIZZLE_W;
>>> -   } else {
>>> -      needsRebase = false;
>>> -   }
>>> +   needsRebase = teximage_needs_rebase(texFormat, baseFormat, true,
>>> +                                       rebaseSwizzle);
>>>
>>>      srcStride = 4 * width * sizeof(GLfloat);
>>>      dstStride = _mesa_image_row_stride(&ctx->Pack, width, format, type);
>>> @@ -423,31 +439,8 @@ get_tex_rgba_uncompressed(struct gl_context *ctx,
>>> GLuint dimensions,
>>>      bool needsRebase;
>>>      void *rgba = NULL;
>>>
>>> -   /* Depending on the base format involved we may need to apply a
>>> rebase
>>> -    * transform (for example: if we download to a Luminance format we
>>> want
>>> -    * G=0 and B=0).
>>> -    */
>>> -   if (texImage->_BaseFormat == GL_LUMINANCE ||
>>> -       texImage->_BaseFormat == GL_INTENSITY) {
>>> -      needsRebase = true;
>>> -      rebaseSwizzle[0] = MESA_FORMAT_SWIZZLE_X;
>>> -      rebaseSwizzle[1] = MESA_FORMAT_SWIZZLE_ZERO;
>>> -      rebaseSwizzle[2] = MESA_FORMAT_SWIZZLE_ZERO;
>>> -      rebaseSwizzle[3] = MESA_FORMAT_SWIZZLE_ONE;
>>> -   } else if (texImage->_BaseFormat == GL_LUMINANCE_ALPHA) {
>>> -      needsRebase = true;
>>> -      rebaseSwizzle[0] = MESA_FORMAT_SWIZZLE_X;
>>> -      rebaseSwizzle[1] = MESA_FORMAT_SWIZZLE_ZERO;
>>> -      rebaseSwizzle[2] = MESA_FORMAT_SWIZZLE_ZERO;
>>> -      rebaseSwizzle[3] = MESA_FORMAT_SWIZZLE_W;
>>> -    } else if (texImage->_BaseFormat !=
>>> -               _mesa_get_format_base_format(texFormat)) {
>>> -      needsRebase =
>>> -
>>> _mesa_compute_rgba2base2rgba_component_mapping(texImage->_BaseFormat,
>>> -                                                        rebaseSwizzle);
>>> -    } else {
>>> -      needsRebase = false;
>>> -    }
>>> +   needsRebase = teximage_needs_rebase(texFormat, texImage->_BaseFormat,
>>> false,
>>> +                                       rebaseSwizzle);
>>>
>>>      /* Describe the dst format */
>>>      dst_is_integer = _mesa_is_enum_format_integer(format);
>>> --
>>> 2.5.0
>>>
>>
>> Anyone want to review this old patch? It still applies cleanly on master.
>
>
> Reviewed-by: Brian Paul <brianp at vmware.com>

Thanks for the review Brian. I'll push the patch with suggested changes.