1444430 - Add compositor-agnostic infrastructure to submit screenshots as profiler markers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement)

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1444430 - Remove a copy in EncodeSourceSurfaceInternal.

Review commit: https://reviewboard.mozilla.org/r/231402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231402/

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1444430 - Extend the API of gfxUtils::EncodeSourceSurface to support encoding into a string.

Review commit: https://reviewboard.mozilla.org/r/231404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231404/

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1444430 - Add a profiler feature called "screenshots".

Review commit: https://reviewboard.mozilla.org/r/231406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231406/

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1444430 - Add a ProfilerScreenshots class which can be used to submit DataSourceSurfaces to the profiler.

ProfilerScreenshots encodes these surfaces to JPG data URLs, and submits them
as profiler markers. The encoding is done on a separate thread.

Review commit: https://reviewboard.mozilla.org/r/231408/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231408/

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8962587 [details]
Bug 1444430 - Remove a copy in EncodeSourceSurfaceInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231402/diff/1-2/

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8962588 [details]
Bug 1444430 - Extend the API of gfxUtils::EncodeSourceSurface to support encoding into a string.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231404/diff/1-2/

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8962589 [details]
Bug 1444430 - Add a profiler feature called "screenshots".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231406/diff/1-2/

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8962590 [details]
Bug 1444430 - Add a ProfilerScreenshots class which can be used to submit DataSourceSurfaces to the profiler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231408/diff/1-2/

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8962587 [details]
Bug 1444430 - Remove a copy in EncodeSourceSurfaceInternal.

https://reviewboard.mozilla.org/r/231402/#review237308

::: gfx/thebes/gfxUtils.cpp:947
(Diff revision 1)
>  EncodeSourceSurfaceInternal(SourceSurface* aSurface,
>                             const nsACString& aMimeType,
>                             const nsAString& aOutputOptions,
>                             gfxUtils::BinaryOrData aBinaryOrData,
>                             FILE* aFile,
> -                           nsCString* aStrOut)
> +                           nsACString* aStrOut)

This function should really be split up.

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8962589 [details]
Bug 1444430 - Add a profiler feature called "screenshots".

https://reviewboard.mozilla.org/r/231406/#review237344

::: browser/components/extensions/schemas/geckoProfiler.json:37
(Diff revision 2)
>            "restyle",
>            "stackwalk",
>            "tasktracer",
>            "threads",
> -          "trackopts"
> +          "trackopts",
> +          "screenshots"

Please keep this list in alphabetical order.

::: tools/profiler/public/GeckoProfiler.h:137
(Diff revision 2)
>    \
>    /* Have the JavaScript engine track JIT optimizations. */ \
> -  macro(10, "trackopts", TrackOptimizations)
> +  macro(10, "trackopts", TrackOptimizations) \
> +  \
> +  /* Take a snapshot of the window on every composition. */ \
> +  macro(11, "screenshots", Screenshots)

Ditto.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8962587 [details]
Bug 1444430 - Remove a copy in EncodeSourceSurfaceInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231402/diff/2-3/

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8962588 [details]
Bug 1444430 - Extend the API of gfxUtils::EncodeSourceSurface to support encoding into a string.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231404/diff/2-3/

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8962589 [details]
Bug 1444430 - Add a profiler feature called "screenshots".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231406/diff/2-3/

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8962590 [details]
Bug 1444430 - Add a ProfilerScreenshots class which can be used to submit DataSourceSurfaces to the profiler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231408/diff/2-3/

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8962588 [details]
Bug 1444430 - Extend the API of gfxUtils::EncodeSourceSurface to support encoding into a string.

https://reviewboard.mozilla.org/r/231404/#review237990

::: gfx/thebes/gfxUtils.cpp:1111
(Diff revision 3)
>                                BinaryOrData aBinaryOrData,
> -                              FILE* aFile)
> +                              FILE* aFile,
> +                              nsACString* aString)
>  {
>    return EncodeSourceSurfaceInternal(aSurface, aMimeType, aOutputOptions,
> -                                     aBinaryOrData, aFile, nullptr);
> +                                     aBinaryOrData, aFile, aString);

Let's just rename Internal instead.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8962590 [details]
Bug 1444430 - Add a ProfilerScreenshots class which can be used to submit DataSourceSurfaces to the profiler.

https://reviewboard.mozilla.org/r/231408/#review238006

::: gfx/layers/ProfilerScreenshots.cpp:63
(Diff revision 3)
> +
> +  aPopulateSurface(backingSurface);
> +
> +  if (!mThread) {
> +    nsresult rv =
> +      NS_NewNamedThread("ProfScreenshot", getter_AddRefs(mThread));

Please add a common do 'eventually' thread.

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8962587 [details]
Bug 1444430 - Remove a copy in EncodeSourceSurfaceInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231402/diff/3-4/

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8962588 [details]
Bug 1444430 - Extend the API of gfxUtils::EncodeSourceSurface to support encoding into a string.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231404/diff/3-4/

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 8962589 [details]
Bug 1444430 - Add a profiler feature called "screenshots".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231406/diff/3-4/

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 8962590 [details]
Bug 1444430 - Add a ProfilerScreenshots class which can be used to submit DataSourceSurfaces to the profiler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231408/diff/3-4/

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 8962588 [details]
Bug 1444430 - Extend the API of gfxUtils::EncodeSourceSurface to support encoding into a string.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231404/diff/4-5/

Comment 22

[Markus Stange [:mstange]]

Comment on attachment 8962589 [details]
Bug 1444430 - Add a profiler feature called "screenshots".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231406/diff/4-5/

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 8962590 [details]
Bug 1444430 - Add a ProfilerScreenshots class which can be used to submit DataSourceSurfaces to the profiler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231408/diff/4-5/

Comment 24

[Markus Stange [:mstange]]

Comment on attachment 8962590 [details]
Bug 1444430 - Add a ProfilerScreenshots class which can be used to submit DataSourceSurfaces to the profiler.

https://reviewboard.mozilla.org/r/231408/#review238006

> Please add a common do 'eventually' thread.

I filed bug 1450059 about this.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e54db9e52b5
Remove a copy in EncodeSourceSurfaceInternal. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/643af79826b8
Extend the API of gfxUtils::EncodeSourceSurface to support encoding into a string. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/a08a2cbad087
Add a profiler feature called "screenshots". r=njn
https://hg.mozilla.org/integration/autoland/rev/8b6f8d93e9dd
Add a ProfilerScreenshots class which can be used to submit DataSourceSurfaces to the profiler. r=jrmuizel

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4e54db9e52b5
https://hg.mozilla.org/mozilla-central/rev/643af79826b8
https://hg.mozilla.org/mozilla-central/rev/a08a2cbad087
https://hg.mozilla.org/mozilla-central/rev/8b6f8d93e9dd