[Mesa-dev] [PATCH] softpipe: fix seamless cube filtering
Roland Scheidegger
sroland at vmware.com
Fri Oct 11 02:01:38 CEST 2013
Am 10.10.2013 23:12, schrieb Brian Paul:
> On 10/10/2013 11:37 AM, sroland at vmware.com wrote:
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> Fix coord wrapping (and face selection too) in case of edges.
>> Unfortunately, the coord wrapping is way more complicated than what
>> the code did, as it depends on the face and the direction where the
>> texel falls off the face (the logic needed to get this right in fact
>> seems utterly ridiculous).
>> Also fix a bug in (y direction under/overflow) face selection.
>> And get rid of complicated cube corner handling. Just like edge case,
>> the coord wrapping was wrong and it seems very difficult to fix.
>> I'm near certain it can't always work anyway (though ordinary seamless
>> filtering on edge has actually a similar problem but not as severe)
>> because we don't have per-pixel face, hence could have multiple corner
>> texels which would make it very difficult to average the remaining texels
>> correctly. Hence simply pick a texel which would only have fallen off one
>> edge but not both instead, which is not quite accurate but actually I
>> think
>> should be enough to meet OpenGL (but not d3d10) requirements.
>> ---
>> src/gallium/drivers/softpipe/sp_tex_sample.c | 200
>> +++++++++++++++++++-------
>> 1 file changed, 150 insertions(+), 50 deletions(-)
>>
>> diff --git a/src/gallium/drivers/softpipe/sp_tex_sample.c
>> b/src/gallium/drivers/softpipe/sp_tex_sample.c
>> index 8dcc297..b905790 100644
>> --- a/src/gallium/drivers/softpipe/sp_tex_sample.c
>> +++ b/src/gallium/drivers/softpipe/sp_tex_sample.c
>> @@ -608,6 +608,48 @@ get_texel_2d(const struct sp_sampler_view *sp_sview,
>> }
>> }
>>
>> +
>> +/*
>> + * Here's the complete logic (HOLY CRAP) for finding next face and
>> doing the
>> + * corresponding coord wrapping, implemented by get_next_face,
>> + * get_next_xcoord, get_next_ycoord.
>> + * Read like that (first line):
>> + * If face is +x and s coord is below zero, then
>> + * new face is +z, new s is max , new t is old t
>> + * (max is always cube size - 1).
>> + *
>> + * +x s- -> +z: s = max, t = t
>> + * +x s+ -> -z: s = 0, t = t
>> + * +x t- -> +y: s = max, t = max-s
>> + * +x t+ -> -y: s = max, t = s
>> + *
>> + * -x s- -> -z: s = max, t = t
>> + * -x s+ -> +z: s = 0, t = t
>> + * -x t- -> +y: s = 0, t = s
>> + * -x t+ -> -y: s = 0, t = max-s
>> + *
>> + * +y s- -> -x: s = t, t = 0
>> + * +y s+ -> +x: s = max-t, t = 0
>> + * +y t- -> -z: s = max-s, t = 0
>> + * +y t+ -> +z: s = s, t = 0
>> + *
>> + * -y s- -> -x: s = max-t, t = max
>> + * -y s+ -> +x: s = t, t = max
>> + * -y t- -> +z: s = s, t = max
>> + * -y t+ -> -z: s = max-s, t = max
>> +
>> + * +z s- -> -x: s = max, t = t
>> + * +z s+ -> +x: s = 0, t = t
>> + * +z t- -> +y: s = s, t = max
>> + * +z t+ -> -y: s = s, t = 0
>> +
>> + * -z s- -> +x: s = max, t = t
>> + * -z s+ -> -x: s = 0, t = t
>> + * -z t- -> +y: s = max-s, t = 0
>> + * -z t+ -> -y: s = max-s, t = max
>> + */
>> +
>> +
>> /*
>> * seamless cubemap neighbour array.
>> * this array is used to find the adjacent face in each of 4
>> directions,
>> @@ -617,49 +659,101 @@ static const unsigned
>> face_array[PIPE_TEX_FACE_MAX][4] = {
>> /* pos X first then neg X is Z different, Y the same */
>> /* PIPE_TEX_FACE_POS_X,*/
>> { PIPE_TEX_FACE_POS_Z, PIPE_TEX_FACE_NEG_Z,
>> - PIPE_TEX_FACE_NEG_Y, PIPE_TEX_FACE_POS_Y },
>> + PIPE_TEX_FACE_POS_Y, PIPE_TEX_FACE_NEG_Y },
>> /* PIPE_TEX_FACE_NEG_X */
>> { PIPE_TEX_FACE_NEG_Z, PIPE_TEX_FACE_POS_Z,
>> - PIPE_TEX_FACE_NEG_Y, PIPE_TEX_FACE_POS_Y },
>> + PIPE_TEX_FACE_POS_Y, PIPE_TEX_FACE_NEG_Y },
>>
>> /* pos Y first then neg Y is X different, X the same */
>> /* PIPE_TEX_FACE_POS_Y */
>> { PIPE_TEX_FACE_NEG_X, PIPE_TEX_FACE_POS_X,
>> - PIPE_TEX_FACE_POS_Z, PIPE_TEX_FACE_NEG_Z },
>> + PIPE_TEX_FACE_NEG_Z, PIPE_TEX_FACE_POS_Z },
>>
>> /* PIPE_TEX_FACE_NEG_Y */
>> { PIPE_TEX_FACE_NEG_X, PIPE_TEX_FACE_POS_X,
>> - PIPE_TEX_FACE_NEG_Z, PIPE_TEX_FACE_POS_Z },
>> + PIPE_TEX_FACE_POS_Z, PIPE_TEX_FACE_NEG_Z },
>>
>> /* pos Z first then neg Y is X different, X the same */
>> /* PIPE_TEX_FACE_POS_Z */
>> { PIPE_TEX_FACE_NEG_X, PIPE_TEX_FACE_POS_X,
>> - PIPE_TEX_FACE_NEG_Y, PIPE_TEX_FACE_POS_Y },
>> + PIPE_TEX_FACE_POS_Y, PIPE_TEX_FACE_NEG_Y },
>>
>> /* PIPE_TEX_FACE_NEG_Z */
>> { PIPE_TEX_FACE_POS_X, PIPE_TEX_FACE_NEG_X,
>> - PIPE_TEX_FACE_NEG_Y, PIPE_TEX_FACE_POS_Y }
>> + PIPE_TEX_FACE_POS_Y, PIPE_TEX_FACE_NEG_Y }
>> };
>>
>> static INLINE unsigned
>> -get_next_face(unsigned face, int x, int y)
>> +get_next_face(unsigned face, int idx)
>> {
>> - int idx = 0;
>> + return face_array[face][idx];
>> +}
>>
>> - if (x == 0 && y == 0)
>> - return face;
>> - if (x == -1)
>> - idx = 0;
>> - else if (x == 1)
>> - idx = 1;
>> - else if (y == -1)
>> - idx = 2;
>> - else if (y == 1)
>> - idx = 3;
>> +static INLINE unsigned
>
> I think that the return type should be int.
Ah yes you're right.
>
>
>> +get_next_xcoord(unsigned face, unsigned fall_off_index, int max, int
>> xc, int yc)
>> +{
>> + if ((face == 0 && fall_off_index != 1) ||
>> + (face == 1 && fall_off_index == 0) ||
>> + (face == 4 && fall_off_index == 0) ||
>> + (face == 5 && fall_off_index == 0)){
>> + return max;
>> + }
>> + if ((face == 1 && fall_off_index != 0) ||
>> + (face == 0 && fall_off_index == 1) ||
>> + (face == 4 && fall_off_index == 1) ||
>> + (face == 5 && fall_off_index == 1)) {
>> + return 0;
>> + }
>> + if ((face == 4 && fall_off_index >= 2) ||
>> + (face == 2 && fall_off_index == 3) ||
>> + (face == 3 && fall_off_index == 2)){
>> + return xc;
>> + }
>> + if ((face == 5 && fall_off_index >= 2) ||
>> + (face == 2 && fall_off_index == 2) ||
>> + (face == 3 && fall_off_index == 3)){
>> + return max - xc;
>> + }
>> + if ((face == 2 && fall_off_index == 0) ||
>> + (face == 3 && fall_off_index == 1)) {
>> + return yc;
>> + }
>> + else {
>> + /* (face == 2 && fall_off_index == 1) ||
>> + (face == 3 && fall_off_index == 0)) */
>> + return max -yc;
>
> space after -
>
>
>> + }
>> +}
>>
>> - return face_array[face][idx];
>> +static INLINE unsigned
>
> return int
>
>
>> +get_next_ycoord(unsigned face, unsigned fall_off_index, int max, int
>> xc, int yc)
>> +{
>> + if ((fall_off_index <= 1) && (face <= 1 || face >= 4)) {
>> + return yc;
>> + }
>> + if (face == 2 ||
>> + (face == 4 && fall_off_index == 3) ||
>> + (face == 5 && fall_off_index == 2)) {
>> + return 0;
>> + }
>> + if (face == 3 ||
>> + (face == 4 && fall_off_index == 2) ||
>> + (face == 5 && fall_off_index == 3)) {
>> + return max;
>> + }
>> + if ((face == 0 && fall_off_index == 3) ||
>> + (face == 1 && fall_off_index == 2)) {
>> + return xc;
>> + }
>> + else {
>> + /* (face == 0 && fall_off_index == 2) ||
>> + (face == 1 && fall_off_index == 3) */
>> + return max - xc;
>> + }
>> }
>>
>> +
>> +
>> static INLINE const float *
>> get_texel_cube_seamless(const struct sp_sampler_view *sp_sview,
>> union tex_tile_address addr, int x, int y,
>> @@ -668,44 +762,47 @@ get_texel_cube_seamless(const struct
>> sp_sampler_view *sp_sview,
>> const struct pipe_resource *texture = sp_sview->base.texture;
>> unsigned level = addr.bits.level;
>> unsigned face = addr.bits.face;
>> - int new_x, new_y;
>> - int max_x, max_y;
>> - int c;
>> + int new_x, new_y, max_x;
>>
>> max_x = (int) u_minify(texture->width0, level);
>> - max_y = (int) u_minify(texture->height0, level);
>> +
>> + assert(texture->width0 == texture->height0);
>> new_x = x;
>> new_y = y;
>>
>> - /* the corner case */
>> - if ((x < 0 || x >= max_x) &&
>> - (y < 0 || y >= max_y)) {
>> - const float *c1, *c2, *c3;
>> - int fx = x < 0 ? 0 : max_x - 1;
>> - int fy = y < 0 ? 0 : max_y - 1;
>> - c1 = get_texel_2d_no_border( sp_sview, addr, fx, fy);
>> - addr.bits.face = get_next_face(face, (x < 0) ? -1 : 1, 0);
>> - c2 = get_texel_2d_no_border( sp_sview, addr, (x < 0) ? max_x -
>> 1 : 0, fy);
>> - addr.bits.face = get_next_face(face, 0, (y < 0) ? -1 : 1);
>> - c3 = get_texel_2d_no_border( sp_sview, addr, fx, (y < 0) ?
>> max_y - 1 : 0);
>> - for (c = 0; c < TGSI_QUAD_SIZE; c++)
>> - corner[c] = CLAMP((c1[c] + c2[c] + c3[c]), 0.0F, 1.0F) / 3;
>> -
>> - return corner;
>> - }
>> /* change the face */
>> if (x < 0) {
>> - new_x = max_x - 1;
>> - face = get_next_face(face, -1, 0);
>> + /*
>> + * Cheat with corners. They are difficult and I believe because
>> we don't get
>> + * per-pixel faces we can actually have multiple corner texels
>> per pixel,
>> + * which screws things up majorly in any case (as the per spec
>> behavior is
>> + * to average the 3 remaining texels, which we might not have).
>> + * Hence just make sure that the 2nd coord is clamped, will
>> simply pick the
>> + * sample which would have fallen off the x coord, but not y
>> coord.
>> + * So the filter weight of the samples will be wrong, but at
>> least this
>> + * ensures that only valid texels near the corner are used.
>> + */
>> + if (y < 0 || y >= max_x) {
>> + y = CLAMP(y, 0, max_x - 1);
>> + }
>> + new_x = get_next_xcoord(face, 0, max_x -1, x, y);
>> + new_y = get_next_ycoord(face, 0, max_x -1, x, y);
>> + face = get_next_face(face, 0);
>> } else if (x >= max_x) {
>> - new_x = 0;
>> - face = get_next_face(face, 1, 0);
>> + if (y < 0 || y >= max_x) {
>> + y = CLAMP(y, 0, max_x - 1);
>> + }
>> + new_x = get_next_xcoord(face, 1, max_x -1, x, y);
>> + new_y = get_next_ycoord(face, 1, max_x -1, x, y);
>> + face = get_next_face(face, 1);
>> } else if (y < 0) {
>> - new_y = max_y - 1;
>> - face = get_next_face(face, 0, -1);
>> - } else if (y >= max_y) {
>> - new_y = 0;
>> - face = get_next_face(face, 0, 1);
>> + new_x = get_next_xcoord(face, 2, max_x -1, x, y);
>> + new_y = get_next_ycoord(face, 2, max_x -1, x, y);
>> + face = get_next_face(face, 2);
>> + } else if (y >= max_x) {
>> + new_x = get_next_xcoord(face, 3, max_x -1, x, y);
>> + new_y = get_next_ycoord(face, 3, max_x -1, x, y);
>> + face = get_next_face(face, 3);
>> }
>>
>> addr.bits.face = face;
>> @@ -1241,6 +1338,7 @@ img_filter_cube_nearest(struct sp_sampler_view
>> *sp_sview,
>> wrap_nearest_clamp_to_edge(s, width, &x);
>> wrap_nearest_clamp_to_edge(t, height, &y);
>> } else {
>> + /* Would probably make sense to ignore mode and just do edge
>> clamp */
>> sp_samp->nearest_texcoord_s(s, width, &x);
>> sp_samp->nearest_texcoord_t(t, height, &y);
>> }
>> @@ -1524,17 +1622,19 @@ img_filter_cube_linear(struct sp_sampler_view
>> *sp_sview,
>> * For seamless if LINEAR filtering is done within a miplevel,
>> * always apply wrap mode CLAMP_TO_BORDER.
>> */
>> - if (sp_samp->base.seamless_cube_map) {
>> + if (1||sp_samp->base.seamless_cube_map) {
>> + /* Note this is a bit overkill, actual clamping is not required */
>> wrap_linear_clamp_to_border(s, width, &x0, &x1, &xw);
>> wrap_linear_clamp_to_border(t, height, &y0, &y1, &yw);
>> } else {
>> + /* Would probably make sense to ignore mode and just do edge
>> clamp */
>> sp_samp->linear_texcoord_s(s, width, &x0, &x1, &xw);
>> sp_samp->linear_texcoord_t(t, height, &y0, &y1, &yw);
>> }
>>
>> addrj = face(addr, face_id);
>>
>> - if (sp_samp->base.seamless_cube_map) {
>> + if (1||sp_samp->base.seamless_cube_map) {
>> tx0 = get_texel_cube_seamless(sp_sview, addrj, x0, y0, corner0);
>> tx1 = get_texel_cube_seamless(sp_sview, addrj, x1, y0, corner1);
>> tx2 = get_texel_cube_seamless(sp_sview, addrj, x0, y1, corner2);
>>
>
> Maybe add a comment on get_next_xcoord(), etc. But LGTM.
Yes sounds like a good idea. I'd be very happy to use simpler functions
if possible :-).
Roland
> Reviewed-by: Brian Paul <brianp at vmware.com>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list