Closed Bug 1444447 Opened 2 years ago Closed 6 months ago

Capture profiler screenshots in BasicCompositor

Categories

(Core :: Graphics: Layers, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox60 --- wontfix
firefox68 --- fixed

People

(Reporter: mstange, Assigned: barret)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files, 4 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Once the infrastructure from bug 1444430 is in place, we should see if we can capture profiler screenshots from BasicCompositor efficiently enough, or whether we should just disable that feature if compositing is not hardware-accelerated.
Whiteboard: [gfx-noted]
Depends on: 1449641, 1444432
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Comment on attachment 8963362 [details]
Bug 1444447 - Implement AsyncReadbackBuffer Compositor APIs for BasicCompositor.

https://reviewboard.mozilla.org/r/232274/#review237730


Code analysis found 2 defects in this patch:
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/layers/basic/BasicCompositor.cpp:887
(Diff revision 1)
>  }
>  
> +class BasicAsyncReadbackBuffer final : public AsyncReadbackBuffer
> +{
> +public:
> +  BasicAsyncReadbackBuffer(const IntSize& aSize)

Error: Bad implicit conversion constructor for 'basicasyncreadbackbuffer' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/basic/BasicCompositor.cpp:901
(Diff revision 1)
> +  {
> +    mSurface = aSurface;
> +  }
> +
> +protected:
> +  ~BasicAsyncReadbackBuffer() override {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~BasicAsyncReadbackBuffer() override {}
  ^
                                       = default;
Comment on attachment 8963362 [details]
Bug 1444447 - Implement AsyncReadbackBuffer Compositor APIs for BasicCompositor.

https://reviewboard.mozilla.org/r/232274/#review241956


Code analysis found 2 defects in this patch:
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/layers/basic/BasicCompositor.cpp:887
(Diff revision 2)
>  }
>  
> +class BasicAsyncReadbackBuffer final : public AsyncReadbackBuffer
> +{
> +public:
> +  BasicAsyncReadbackBuffer(const IntSize& aSize)

Error: Bad implicit conversion constructor for 'basicasyncreadbackbuffer' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/basic/BasicCompositor.cpp:901
(Diff revision 2)
> +  {
> +    mSurface = aSurface;
> +  }
> +
> +protected:
> +  ~BasicAsyncReadbackBuffer() override {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~BasicAsyncReadbackBuffer() override {}
  ^
                                       = default;
Depends on: 1514307
Assignee: mstange → brennie
Depends on: 1514803
On some platforms we do not always have a DrawTarget that is the size of the
entire window, so we will be unable to record the contents into screenshots in
the profiler output. Now we create an additional DrawTarget that will contain
the contents of the entire window so that we can record it for screenshots.
This only adds the overhead of allocation while profiling and only when
screenshots are requested.

However, it is worth noting that since we are only capturing regions that have
been invalidated, screenshots may be missing some details. For example, the
initial screenshots likely won't have the browser chrome until it is
re-composited, after which it will be included in future screenshots. Sections
of the page will also be missing until they are composited. In both of these
cases, the missing areas will appear black.
Attachment #8963363 - Attachment is obsolete: true
Attachment #8963363 - Flags: review?(jmuizelaar)
Attachment #8963362 - Attachment is obsolete: true
Attachment #8963362 - Flags: review?(jmuizelaar)
Attachment #8963361 - Attachment is obsolete: true
Attachment #8963361 - Flags: review?(jmuizelaar)
Attachment #8963360 - Attachment is obsolete: true
Attachment #8963360 - Flags: review?(jmuizelaar)
Attachment #9032195 - Attachment description: Bug 1444447 - While recording profile screenshots, create a full-window render target and buffer all draws to it r?jrmuizel → Bug 1444447 - While recording profile screenshots, create a full-window render target and buffer all draws to it r?mstange

The current implementation works but we end up with a few blank frames at the beginning. I'll circle back to this.

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a115176a8faa
While recording profile screenshots, create a full-window render target and buffer all draws to it r=mstange
https://hg.mozilla.org/integration/autoland/rev/28e336f7b9da
Implement AsyncReadbackBuffer Compositor APIs for BasicCompositor r=mstange
https://hg.mozilla.org/integration/autoland/rev/a70ed4f3086a
Implement Compositor::BlitRenderTarget for BasicCompositor r=mstange

Keywords: checkin-needed

I'm going to spin off the blank frames issue into a new bug.

Keywords: leave-open
Blocks: 1545262
Pushed by brennie@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/643f81697dae
Correctly copy drawn surfaces to the full window render target in BasicCompositor r=mstange

This causes corruption of library and hamburger menus on at least some win10/64 systems.

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Thanks Bill, I've asked for a backout of that changeset. Indeed, there is a mixup of the offsets in the part of the patch that touches non-profiler code.

Backed out because it causes graphical corruption in menus, see comment 25

Backout: https://hg.mozilla.org/mozilla-central/rev/2ccc6648064315964dd23039ad28ebf7d9f82999

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla68 → ---
Flags: needinfo?(brennie)
No longer blocks: 1545608
Regressions: 1545608
Pushed by brennie@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/040c255450c9
Correctly copy drawn surfaces to the full window render target in BasicCompositor r=mstange
Flags: needinfo?(brennie)
Status: REOPENED → RESOLVED
Closed: 6 months ago6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.