Closed Bug 1023473 Opened 7 years ago Closed 7 years ago

Flicker at bottom of settings app when overscrolled

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34
blocking-b2g 2.0+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: botond, Assigned: jrmuizel)

References

Details

Attachments

(3 files, 8 obsolete files)

STR:
  1. Run B2G with overscrolling enabled (default since this week, otherwise
     can be turned on in developer prefs).
  2. Open the settings app and scroll to the bottom.
  3. Overscroll at the bottom, holding your finger down to keep the page
     overscrolled.

A line of pixels at the bottom of the page contents (under the "Help" item) flickers on and off. This looks bad.
I don't see this on either a Hamachi running 2.0 or a Flame running 2.1. I do see a line on the right edge of the content that varies between 1 and 2 pixels thick, it looks like. That might just be a scaling artifact that we can't do much about though.
blocking-b2g: --- → 2.0?
Assignee: nobody → botond
The video recording is not from my phone - I do see this issue on my phone, but not as much as is in the video.  In fact, probably wouldn't have seen it had I not known to look for it.  Trunk 7/2, latest firmware
Assignee: botond → jmuizelaar
blocking-b2g: 2.0? → 2.0+
This shrinks the texture coords when we're drawing the thing scaled. This should get rid of the artifact but might make things snap to blurry.
Attached patch Pad the borders of tiles (obsolete) — Splinter Review
This is another approach. This padding code just pads the bounds (and not the corners). Doing this padding correctly for an arbitrary region is somewhat tricky.
Attached patch A more clever version (obsolete) — Splinter Review
Attachment #8450995 - Attachment is obsolete: true
Attachment #8451106 - Attachment is obsolete: true
Attachment #8450997 - Attachment is obsolete: true
Comment on attachment 8451107 [details] [diff] [review]
A more clever version

Review of attachment 8451107 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/TiledContentHost.cpp
@@ +384,5 @@
> +        effect->mTextureCoords = Rect((textureRect.x ? textureRect.x+0.5f : 0) / aTextureBounds.width,
> +                                      (textureRect.y ? textureRect.y+0.5f : 0) / aTextureBounds.height,
> +                                      (textureRect.width == aTextureBounds.width ? textureRect.width : textureRect.width-(textureRect.x ? 1.f : 0.5f)) / aTextureBounds.width,
> +                                      (textureRect.height == aTextureBounds.height ? textureRect.height : textureRect.height-(textureRect.y ? 1.f : 0.5f)) / aTextureBounds.height);
> +    }

By now, width and height are guaranteed > 0?

In general, the effect you're going for is to effectively "shrink" the coordinates to avoid the outside "ring" on the texture?
Attachment #8451135 - Attachment is obsolete: true
Attachment #8453046 - Attachment is obsolete: true
This is still a little rough around the edges but the basic approach is there. Don't worry too much about cosmetics during the review.
Attachment #8453292 - Attachment is obsolete: true
Attachment #8453845 - Flags: review?(bgirard)
Comment on attachment 8453845 [details] [diff] [review]
A slightly cleaned up version of the padding approach

Review of attachment 8453845 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/nsRegion.cpp
@@ +372,5 @@
>  
> +typedef void (*visit_fn)(void *closure, int side, int x1, int y1, int x2, int y2);
> +
> +static void VisitRects(visit_fn visit, void *closure,
> +		       int side, pixman_box32_t *&r1, pixman_box32_t *&r2, const int y, int &x1)

Comment or assert that r1->x1 <= r2->x1 to help the future readers/users?
Comment on attachment 8453845 [details] [diff] [review]
A slightly cleaned up version of the padding approach

Review of attachment 8453845 [details] [diff] [review]:
-----------------------------------------------------------------

A few comments have already been voided based on our discussion in person, feel free to ignore them.

::: gfx/layers/client/TiledContentClient.cpp
@@ +841,5 @@
> +	if (y1 < height) {
> +	  memcpy(&bitmap[x1*bpp + y1 * stride], &bitmap[x1*bpp + (y1-1) * stride], (x2 - x1) * bpp);
> +	}
> +      } else if (side == 2) {
> +	if (x1 > 0) while (y1 != y2) {

while should be on a different line. It's too easy to miss that the if is actually a loop. If you want this pattern then while (x1 > 0 && y1 !+ y2) is better. Likewise below.

@@ +842,5 @@
> +	  memcpy(&bitmap[x1*bpp + y1 * stride], &bitmap[x1*bpp + (y1-1) * stride], (x2 - x1) * bpp);
> +	}
> +      } else if (side == 2) {
> +	if (x1 > 0) while (y1 != y2) {
> +	  memcpy(&bitmap[(x1-1)*bpp + y1 * stride], &bitmap[x1*bpp + y1*stride], bpp);

Is using a memcpy a good idea for one pixel?

@@ +857,5 @@
> +  } lb;
> +  if (drawTarget->LockBits(&lb.data, &lb.size, &lb.stride, &lb.format)) {
> +    region.VisitEdges(lb.visitor, &lb);
> +    drawTarget->ReleaseBits(lb.data);
> +  }

needs a comment to explain that we just ignore this if we can't get the data.

@@ +937,5 @@
>        // Mark the newly updated area as invalid in the front buffer
>        aTile.mInvalidFront.Or(aTile.mInvalidFront, nsIntRect(copyTarget.x, copyTarget.y, copyRect.width, copyRect.height));
>      }
>  
> +    // only worry about padding when not doing low-res

// the artifacts shouldn't be noticeable on the low-res data.

@@ +944,5 @@
> +					 aTileOrigin.y,
> +					 GetTileSize().width,
> +					 GetTileSize().height);
> +
> +      nsIntRegion tileValidRegion = GetValidRegion();

Can we move this code into a private method? We should explain the code why were doing this. To an outsider they might wonder why we do all this extra work.

::: gfx/src/nsRegion.cpp
@@ +371,5 @@
>  
>  
> +typedef void (*visit_fn)(void *closure, int side, int x1, int y1, int x2, int y2);
> +
> +static void VisitRects(visit_fn visit, void *closure,

x1 should probably just be the return value.

@@ +372,5 @@
>  
> +typedef void (*visit_fn)(void *closure, int side, int x1, int y1, int x2, int y2);
> +
> +static void VisitRects(visit_fn visit, void *closure,
> +		       int side, pixman_box32_t *&r1, pixman_box32_t *&r2, const int y, int &x1)

That's a good point. This should be documented in nsRegion.h

@@ +407,5 @@
> +#define BOTTOM 1
> +#define LEFT 2
> +#define RIGHT 3
> +
> +//XXX: if we need to this can compute the end of the row

You should decide if you want to do this or not. It shouldn't be left as a 'XXX' warning.

@@ +412,5 @@
> +static void
> +VisitSides(visit_fn visit, void *closure, pixman_box32_t *r, pixman_box32_t *r_end)
> +{
> +  // XXX: we can drop LEFT/RIGHT and just use the orientation
> +  // of the line if it makes sense

Let's remove this. I think using the order of the point to encode the direction is very confusing.

@@ +446,5 @@
> +               pixman_box32_t *r2,
> +               pixman_box32_t *r2_end)
> +{
> +  const int y = r1->y2;
> +  int x1;

Maybe lastXVisited would be a better name (or something similar).

@@ +448,5 @@
> +{
> +  const int y = r1->y2;
> +  int x1;
> +
> +  /* Find the left-most edge */

Not a fan of block comments for single line comments.

@@ +458,5 @@
> +
> +  while (r1 != r1_end && r2 != r2_end) {
> +    MOZ_ASSERT((x1 >= (r1->x1 - 1)) || (x1 >= (r2->x1 - 1)));
> +    if (r1->x1 < r2->x1) {
> +      VisitRects(visit, closure, BOTTOM, r1, r2, y, x1);

VisitNextEdgeBetweenRect might be a better name since every iteration here you visit the next edge.

If you rename x1 and move it as the return value this would become
lastXVisited = VisitNextEdgeBetweenRect(...)

@@ +470,5 @@
> +    // top row
> +    do {
> +      visit(closure, BOTTOM, x1, y, r1->x2 + 1, y);
> +      r1++;
> +      // 

I feel like you had something important to tell me here and you left me hanging.

::: gfx/src/nsRegion.h
@@ +258,5 @@
>     * original region.
>     */
>    void SimplifyInward (uint32_t aMaxRects);
>  
> +  // XXX: side should be an enum

Can we add that in before landing it? Or maybe we just want to use deltaX/deltaY to avoid the conversion to direction, then direction back to offsets.

Also VisitEdge should have very clear documentation. Please mention if the visit_fn ever covers the same region twice or not.

You should document the callback sent to visit_fn. State the definition of the line you pass an as argument.

::: gfx/tests/gtest/TestRegion.cpp
@@ +171,5 @@
>    }
>  
>  }
>  
> +

useless newline

@@ +318,5 @@
> +	if (bitmap[x + y*width] == REGION_VALUE) {
> +	  for (int yn = max(y-1, 0); yn <= min(y+1, height-1); yn++) {
> +	    for (int xn = max(x-1, 0); xn <= min(x+1, width-1); xn++) {
> +	      if (bitmap[xn + yn*width] == 0)
> +		bitmap[xn + yn*width] = DILATE_VALUE;

We should have a space between the operator(s). This is inconsistent even within the patch.

@@ +334,5 @@
> +    }
> +  }
> +};
> +
> +void visit(void *closure, int side, int x1, int y1, int x2, int y2)

Below I suggest clearer code and suggest calling it VisitEdge.

@@ +342,5 @@
> +  const int width = visitor->width;
> +
> +  if (side == 0) {
> +    while (x1 != x2) {
> +      bitmap[x1 + (y1-1) * width] = DILATE_VALUE;

If you guarantee that the same area wont be covered twice you should assert this here by checking that the previously value was 0. Otherwise please document the behavior clearly in VisitEdge.

@@ +363,5 @@
> +    }
> +  }
> +}
> +
> +void TestVisit(nsRegion &r)

This is really confusing. IMO You visitor should hide the bitmap and you should have a compare function between the two Visitor. You can rename Visitor to RegionBitmap. The code would instead look like:

RegionBitmap ref(r);
b.dilate();

RegionBitmap result(r);
r.VisitEdge(SetEdge, result);

ref.compare(result);

@@ +381,5 @@
> +  r.VisitEdges(visit, &v);
> +  v.compare(reference);
> +}
> +
> +TEST(Gfx, RegionVisitEdges) {

It's best to split these up into different tests. instead of using local scope. Also including a RegionVisitEdgeTrivial that runs first to check the sanity of the code before running more complex test cases.

You spent some time to lay down the helpers for testing but you didn't write many cases using this infrastructure.

IMO you should test the following:
- Trivial rect
- Two rect far apart (horizontal, vertical)
- Two rects one pixel apart (hozirontal, vertical)
- One rect with two rects below it
- Two rect touching at a corner
- Two rect touching one pixel away from the corner
- A multiline (3 or more) test case
- I'm not even sure if the above would still hit most branches in your code. Might be good to make sure not leaving a lot of branches untested.

@@ +397,5 @@
> +    TestVisit(r);
> +  }
> +
> +  {
> +    // no touching

LOL, not*
Attachment #8453845 - Flags: review?(bgirard) → review-
Attached patch Slightly updated (obsolete) — Splinter Review
Attachment #8453845 - Attachment is obsolete: true
Attachment #8457455 - Attachment is obsolete: true
Attachment #8458883 - Flags: review?(bgirard)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> Created attachment 8458883 [details] [diff] [review]
> With review comments applied

I also fixed some over-read bugs.
Comment on attachment 8458883 [details] [diff] [review]
With review comments applied

Review of attachment 8458883 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/nsRegion.h
@@ +42,5 @@
> +	TOP,
> +	BOTTOM,
> +	LEFT,
> +	RIGHT
> +MOZ_END_ENUM_CLASS(VisitSide)

You decided to keep the direction and not use the deltaX/deltaY?

@@ +270,5 @@
> +   * VisitEdges is a weird kind of function that we use for padding
> +   * out surfaces to prevent texture filtering artifacts.
> +   * It calls the visitFn callback for each of the exterior edges of
> +   * the regions. The top and bottom edges will be expanded 1 pixel
> +   * to the left and right if there's an outside corner. The order

I'm not sure what you mean by outside corner. A region will always have outside corners.
Attachment #8458883 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/7a5b3439faa7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1045122
Depends on: 1049138
Depends on: 1073554
See Also: → 1126644
You need to log in before you can comment on or make changes to this bug.