927954 - Resurrect the checkerboard visualization tool

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Back when we were trying to reduce checkerboarding in Native Fennec, we had a tool that would help us visualize what was going on. The code from that is in my github repo at 

https://github.com/staktrace/andreasgal-checkviz

Basically the way it worked is we would instrument the gecko code to print out various rectangles at various points (at frame composite time and at repaint time), and collect that output into a file. Then we could run the server.js file in Node, which would serve up the client.html/client.js files. Loading that file in the browser would show a nice animation of what was happening. It was very easy to see when the displayport was far away from the viewport and so you would have checkerboarding.

Comment 1

[Botond Ballo [:botond]]

I'll have a look at this in the hope that it will be helpful for tracking down what we think are displayport issues on Metro.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Taking this from Botond.

Also for future reference the old patch with Checkviz logging is at https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=747528&attachment=618012

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

So in retrospect the tool I was actually thinking of was rendertrace (https://github.com/staktrace/rendertrace) rather than checkviz. I could have sworn we did rendertrace as an improvement on checkviz but the github commits say otherwise. Anyway, this patch adds the right logging for rendertrace, but I kept running into bug 937274. I'll apply the patch from that and re-test.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Fixed a colour and the size of the composition bounds.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 830891 [details] [diff] [review]
Patch

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +74,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +#ifdef APZC_ENABLE_RENDERTRACE
> +  static TimeStamp sRenderStart = TimeStamp::Now();

Will this ever be reset? If not, perhaps we make it const.

@@ +79,5 @@
> +
> +  #define APZC_LOG_RENDERTRACE_RECT(prefix, color, rect) \
> +    printf_stderr(prefix " RENDERTRACE %f rect " color " %f %f %f %f\n", \
> +                  (TimeStamp::Now() - sRenderStart).ToMilliseconds(), \
> +                  (rect).x, (rect).y, (rect).width, (rect).height);

I would prefer keeping code out of macro bodies as much as possible. Can we do something like:

static inline apzc_log_rendertrace_rect(const char* prefix, const char* color, const CSSRect& rect)
{
    nsString msg(prefix);
    msg.AppendPrintf(" RENDERTRACE %f rect ", (TimeStamp::Now() - sRenderStart).ToMilliseconds());
    msg.Append(color);
    msg.AppendPrintf(" %f %f %f %f\n", rect.x, rect.y, rect.width, rect.height));
    printf_stderr(msg.get());
}

#define APZC_LOG_RENDERTRACE_RECT apzc_log_rendertrace_rect

instead?

(In fact, APZC_LOG_FM could be refactored in a similar way as well.)

@@ +540,5 @@
>  }
>  
>  nsEventStatus AsyncPanZoomController::OnTouchEnd(const MultiTouchInput& aEvent) {
>    APZC_LOG("%p got a touch-end in state %d\n", this, mState);
> +

Stray newline? (Ok if intentional.)

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

How about this? It's similar to yours but a bit more compact. Also I got rid of the stray newline, it was left over from a previous version of the patch.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 830979 [details] [diff] [review]
Patch v2

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

Note that with this approach, the expression passed in for the rect will be evaluated even if APZC_LOG_RENDERTRACE is not defined. (A good optimizing compiler will eliminate that computation as dead code, but that's still work for the compiler to do.) Fine with me, just wanted to point it out.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #7)
> Note that with this approach, the expression passed in for the rect will be
> evaluated even if APZC_LOG_RENDERTRACE is not defined. (A good optimizing
> compiler will eliminate that computation as dead code, but that's still work
> for the compiler to do.) Fine with me, just wanted to point it out.

Hm, that's a good point that I hadn't considered. I'm ok with it though since as you said the compiler should kill off that code. At least it should once I land https://hg.mozilla.org/try/rev/ca49d069e36c

Comment 9

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> At least it should
> once I land https://hg.mozilla.org/try/rev/ca49d069e36c

While we're on the topic of making things const, I think nsLayoutUtils::FindIDFor could take a const nsIContent* rather than an nsIContent* (and then so could your GetScrollIdentifiers).

Comment 10

[Kartikaya Gupta (email:[e-mail])]

I filed bug 937843 for const'ing assorted things.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3be0a2467fa8

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3be0a2467fa8