Closed Bug 1022080 Opened 7 years ago Closed 6 years ago

MOZ_DUMP_PAINT=1 debugging code interferes with production code, makes reftest fail, doesn't provide useful info

Categories

(Core :: Graphics: Layers, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(5 files, 1 obsolete file)

1. On a linux machine, modify layout/reftest/reftest-sanity/reftest.list so that only the last two tests (async-scroll-1a.html and async-scroll-1b.html) are uncommented.
2. Run EXTRA_TEST_ARGS="--setpref=layers.dump=true --setpref=layers.async-pan-zoom.enabled=true" ./mach reftest-ipc layout/reftests/reftest-sanity
3. Observe test async-scroll-1a passes (1b might fail, see bug 1017065)
4. Run MOZ_DUMP_PAINT=1 EXTRA_TEST_ARGS="--setpref=layers.dump=true --setpref=layers.async-pan-zoom.enabled=true" ./mach reftest-ipc layout/reftests/reftest-sanity
5. Observe test async-scroll-1a fails. The "actual" image is all yellow.
I think we force intermediate surfaces and a few other things that are different when dump painting is enabled.
Indeed, and it totally threw me off track when debugging bug 1017065. Shouldn't the behaviour be the same with and without intermediate surfaces?
Assignee: nobody → bugmail.mozilla
Blocks: 1151617
Summary: MOZ_DUMP_PAINT=1 debugging code interferes with production code, makes reftest fail → MOZ_DUMP_PAINT=1 debugging code interferes with production code, makes reftest fail, doesn't provide useful info
So that setting MOZ_DUMP_PAINT=1 doesn't interfere with the production code.
Attachment #8603657 - Flags: review?(nical.bugzilla)
Whoops, wrong file
Attachment #8603657 - Attachment is obsolete: true
Attachment #8603657 - Flags: review?(nical.bugzilla)
Attachment #8603658 - Flags: review?(nical.bugzilla)
Not sure what this DebugPaintItems stuff is supposed to be doing, but for me it hangs the browser. Moving it to a separate variable so I don't have to worry about it.
Attachment #8603662 - Flags: review?(matt.woodrow)
Attachment #8603662 - Flags: review?(matt.woodrow) → review+
Attachment #8603658 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8603659 [details] [diff] [review]
Part 2 - Make plaintext dumping work better

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

::: gfx/layers/Layers.cpp
@@ +1426,5 @@
>  {
>    nsCString string(aObj->Name());
>    string.Append('-');
>    string.AppendInt((uint64_t)aObj);
> +  if (gfxUtils::sDumpPaintFile != stderr) {

What's the intent here?
Attachment #8603660 - Flags: review?(nical.bugzilla) → review+
Attachment #8603661 - Flags: review?(nical.bugzilla) → review+
(In reply to Nicolas Silva [:nical] from comment #9)
> Comment on attachment 8603659 [details] [diff] [review]
> Part 2 - Make plaintext dumping work better
> 
> Review of attachment 8603659 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Layers.cpp
> @@ +1426,5 @@
> >  {
> >    nsCString string(aObj->Name());
> >    string.Append('-');
> >    string.AppendInt((uint64_t)aObj);
> > +  if (gfxUtils::sDumpPaintFile != stderr) {
> 
> What's the intent here?

To stop dumping html-specific debugging output when dumping a plaintext layers dump to stderr. If you look at the history of the code I'm pretty sure this was an accidental breakage. sDumpPaintFile used to be initialized to nullptr and then it was changed to be initialized to stderr, and so now this html-specific debugging output always shows up.
Attachment #8603659 - Flags: review?(nical.bugzilla) → review+
You need to log in before you can comment on or make changes to this bug.