All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=825519

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=8a666f0d83cdc60cac0b7e69157f82b5187f55afcc72105d8180fed78f569559


Bot(s) for this bug's original alert(s):

android-nexus5
android-nexus5X
android-nexus6
android-nexus7v2
android-one
chromium-rel-win7-gpu-ati
chromium-rel-win7-gpu-intel
chromium-rel-win7-gpu-nvidia

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/168156ad440000

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/168156ad440000

Change how VideoResourceUpdater allocates resources. by kylechar@chromium.org
https://chromium.googlesource.com/chromium/src/+/df9d6f301f2f79230b215d32ec9b2fe227e849dd

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Seems like I broke something with memory dumps for the hardware plane allocations in VideoResourceUpdater. I don't understand exactly what/how it got broken because the numbers don't add up the way I would expect for double counting.

The renderer has the same memory usage characteristics before/after after my patch. It looks like six PlaneResources are being allocated.

2x945kb + 4x236.3kb = 2.8mb

The category changed from "cc/resource_memory/provider_N/resource_M" to "cc/video_memory/updater_N/resource_M" which was expected. This shows that VideoResourceUpdater is creating the same number of PlaneResource objects and they have the same size, so that's good.

The difference is that after my patch the browser process cc went from reporting using 0mb to 5.5mb. Under "cc/resource_memory/provider_N/resource_M" there are the same number of entries before/after my patch. Three entries didn't report a size at all before but after reported something. There are two 945kb resources which correspond to two video plane resources. What I would expect is another four 236.3kb resources, corresponding to the other four video plane resources, but instead there is one 3780kb resource. I'm not really sure what that means so I'll have to investigate.

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/168f2e33440000

I'm looking at before after traces from here: https://pinpoint-dot-chromeperf.appspot.com/job/168156ad440000

The renderer reported "cc/video_memory/updater_N/resource_M" resources are linked by GUID with GPU "gpu/gl/textures/shared_group_N/texture_M" resources both before/after my patch, so I didn't break that.

The browser resources are similarly linked to GPU resources, except a different shared group than the renderer resources. There are 8 resources reported in the browser under "cc/resource_memory/provider_N/resource_M". Before my patch 5 of 8 had non-zero size, had a 0 effective size and were linked to GPU resources. It looks like the GPU process agrees on the size of these resources. The other 3 resources only have a name, no size and aren't linked to anything in the GPU.

After my patch those 3 resources have a non-zero size, the same effective size and are linked to something in the GPU. The reported size in the browser is four times larger than the reported size in the GPU.

In the before trace the three browser cc resources with missing size are linked to GPU resources after all, it just doesn't say nex to the entry with no size. There is one 945kb and two 236.3kb resources listed in the GPU and they have a link back to the browser cc resources I would expect.

I think this means two things. Before my patch, when DisplayResourceProvider is dumping memory the video plane resources had an empty size and didn't go into this if statement:

https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?l=231

After my patch, the video plane resources hit had a size and did go into that block. The size is off by a factor of four, so the resource format is probably wrong.

📍 Couldn't reproduce a difference.
https://pinpoint-dot-chromeperf.appspot.com/job/168f2e33440000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/16d0ee23440000

📍 Couldn't reproduce a difference.
https://pinpoint-dot-chromeperf.appspot.com/job/16d0ee23440000

> Three entries didn't report a size at all before but after reported something.

They didn't have a size or they didn't have a size owned by the browser? ie is this a question of sizes changing or ownership linkage breaking?

> There are two 945kb resources which correspond to two video plane resources. What I would expect is another four 236.3kb resources, corresponding to the other four video plane resources, but instead there is one 3780kb resource. I'm not really sure what that means so I'll have to investigate.

Did you sort out what the 3780kb resource is?

> They didn't have a size or they didn't have a size owned by the browser? ie is this a question of sizes changing or ownership linkage breaking?

DisplayResourceProvider didn't have a size for them before my change. The renderer LayerTreeFrameSink before and VideoResourceUpdater after did have the correct size. The GPU texture also has the correct size.

> Did you sort out what the 3780kb resource is?

It's a RED_8 1344x720 plane resource that DisplayResourceProvider thinks is RGBA_8888 1344x720.

I've the differences mostly tracked down:

1. The reason why DisplayResourceProvider now has a size is because I changed TransferableResource::MakeGL() into TransferableResource::MakeGLOverlay(), the later of which sets the size. DisplayResourceProvider now has a size to report in OnMemoryDump().
2. We don't set the resource format on the TransferableResource which is why it's wrong when it arrives DisplayResourceProvider. This wasn't a problem before because of (1).
3. Setting the format correctly fixes the size in DisplayResourceProvider.

There is still the issue of memory being double counted. This happens because VideoResourceUpdater and DisplayResourceProvider have ContextProviders in different share groups. They produce a different MemoryAllocatorDumpGuid for the same |texture_id|.

> There is still the issue of memory being double counted. This happens because VideoResourceUpdater and DisplayResourceProvider have ContextProviders in different share groups. They produce a different MemoryAllocatorDumpGuid for the same |texture_id|.

Hmmm, and yet tile memory isn't double counted but it has the same situation? Maybe ericrk can help us understand

The GPU texture memory dump adds an extra edge to the memory graph based on the service side texture id. That happens here:

https://cs.chromium.org/chromium/src/gpu/command_buffer/service/texture_manager.cc?l=3542&rcl=a783ac29716b05560ef605df0e9d2665ab3010bd

For the things that are double counted they have the same texture id returned by ref->texture()->service_id(). The textures are in different TextureManagers with different ShareGroupTracingGUID() though so that doesn't help. I don't really understand what the intention of this extra edge is, so I'm not sure if this is the problem.

Re #14 - Tile memory should also be double-counted (if exported to the browser proc). Aliasing between renderer/gpu and browser/gpu works, but the second set of edges that's supposed to address renderer/browser aliasing is currently broken.

issue 571026 is filed to address this. That bug has been taking a while to resolve, so we *may* want to tweak things in the meantime if this is causing significant issues.

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14da25bb440000

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/14da25bb440000

Make VideoResourceUpdater set formats. by kylechar@chromium.org
https://chromium-review.googlesource.com/c/chromium/src/+/982501/6

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14b2d98b440000

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/14b2d98b440000

Make VideoResourceUpdater set formats. by kylechar@chromium.org
https://chromium-review.googlesource.com/c/chromium/src/+/982501/6

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db3a6f73062b72e41fd2a00e330c2048f6e259e0

commit db3a6f73062b72e41fd2a00e330c2048f6e259e0
Author: kylechar <kylechar@chromium.org>
Date: Wed Mar 28 16:51:48 2018

Make VideoResourceUpdater set formats.

This CL changes VideoResourceUpdater to set the ResourceFormat and
BufferFormat on TransferableResources it is responsible for. This caused
memory accounting issues where DisplayResourceProvider had the wrong
size. For example, DisplayResourceProvider thought a resource was
RGBA_888 instead of RED_8 so the reported size was 4x what it should
have been.

This CL doesn't fix the double counting of video plane resources from
VideoResourceUpdater and DisplayResourceProvider. The memory dump infra
is missing functionality needed to do this. See https://crbug.com/571026
for more details on the double counting problem.

Bug:  825519 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ic6c61e492ded7708dc96194510d27966b871a4a1
Reviewed-on: https://chromium-review.googlesource.com/982501
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546524}
[modify] https://crrev.com/db3a6f73062b72e41fd2a00e330c2048f6e259e0/cc/resources/video_resource_updater.cc
[modify] https://crrev.com/db3a6f73062b72e41fd2a00e330c2048f6e259e0/services/viz/public/interfaces/compositing/transferable_resource.mojom

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/16855587440000

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/16855587440000

Make VideoResourceUpdater set formats. by kylechar@chromium.org
https://chromium.googlesource.com/chromium/src/+/db3a6f73062b72e41fd2a00e330c2048f6e259e0

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

I've fixed the reporting in DisplayResourceProvider so the size is correct. There is a corresponding drop in cc memory usage seen here.

https://chromeperf.appspot.com/group_report?sid=8a666f0d83cdc60cac0b7e69157f82b5187f55afcc72105d8180fed78f569559

I can't fix the double counting of memory from VideoResourceUpdater and DisplayResourceProvider from #16 so there is still a (smaller) reported increase. It looks like crbug.com/571026 is relevant bug to follow for that.