1360001 - Use BlobImages for PaintedLayers

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP

This roughly works but has known problems.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add back some removed code for in-memory recording surfaces

This just adds back some code that was removed recently.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add a painted layer that uses blob support

This is not very finished but I figured it would be better to get it in sooner rather than later. You can review it more thoroughly if you want but I haven't even gone through it much yet, so it might be better to do a post review later once it's in better shape.

Comment 4

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

Comment on attachment 8862712 [details] [diff] [review]
Add a painted layer that uses blob support

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

Looks ok mostly, but enough things that I'd like to see a new version. Also add a real commit message.

::: gfx/layers/moz.build
@@ +407,5 @@
>      'wr/WebRenderLayer.cpp',
>      'wr/WebRenderLayerManager.cpp',
>      'wr/WebRenderLayersLogging.cpp',
>      'wr/WebRenderPaintedLayer.cpp',
> +    'wr/WebRenderPaintedLayerBlob.cpp',

Should add the .h file to moz.build as well

::: gfx/layers/wr/WebRenderLayerManager.cpp
@@ +501,5 @@
>  {
> +    if (gfxPrefs::WebRenderBlobImages()) {
> +        return MakeAndAddRef<WebRenderPaintedLayerBlob>(this);
> +    } else {
> +        return MakeAndAddRef<WebRenderPaintedLayer>(this);

2-space indent

::: gfx/layers/wr/WebRenderPaintedLayerBlob.cpp
@@ +11,5 @@
> +#include "mozilla/layers/WebRenderBridgeChild.h"
> +#include "mozilla/layers/UpdateImageHelper.h"
> +#include "mozilla/webrender/WebRenderTypes.h"
> +#include "gfxPrefs.h"
> +#include "gfxUtils.h"

Move gfxPrefs.h and gfxUtils.h up to above LayersLogging.h

@@ +21,5 @@
> +
> +void
> +WebRenderPaintedLayerBlob::RenderLayer(wr::DisplayListBuilder& aBuilder)
> +{
> +  StackingContextHelper sc(aBuilder, this);

This will actually push a stacking context. It seems undesirable to do in the cases where we early-return, so I think you should move this down until after you've done all the early returns. Probably to just before where you do `LayerRect rect = Bounds();` for consistency with the other classes.

@@ +40,5 @@
> +    return;
> +  }
> +
> +  if (!regionToPaint.IsEmpty() && Manager()->GetPaintedLayerCallback()) {
> +  } else {

Is there going to be anything inside the "if" part of this? If so add a placeholder comment for it. If not, just invert the condition and move the "else" body into the "if" half.

@@ +47,5 @@
> +    MOZ_ASSERT(mImageContainer->HasCurrentImage());
> +    MOZ_ASSERT(GetInvalidRegion().IsEmpty());
> +  }
> +
> +  LayerIntRegion visibleRegion = GetVisibleRegion();

Move `visibleRegion` declaration up to the top and use it in the IsEmpty() check

@@ +55,5 @@
> +  if (size.IsEmpty()) {
> +      if (gfxPrefs::LayersDump()) {
> +        printf_stderr("PaintedLayer %p skipping\n", this->GetLayer());
> +      }
> +      return;

2-space indent. Also you might want to modify the error message to distinguish it from the other one at the top of the function. Although really it seems redundant, because if the bounds are empty then the visibleregion must have been empty and that would have been caught by the check at the top, no? I don't see anything that modifies the visible region in between.

@@ +57,5 @@
> +        printf_stderr("PaintedLayer %p skipping\n", this->GetLayer());
> +      }
> +      return;
> +  }
> +  IntSize imageSize(size.width, size.height);

I'd rather use size.ToUnknownSize() to make it more explicit we're going into untyped-land.

@@ +64,5 @@
> +  RefPtr<gfx::DrawTarget> dt = gfx::Factory::CreateRecordingDrawTarget(recorder, dummyDt);
> +
> +
> +
> +  {

3 blank lines is too many

::: gfx/layers/wr/WebRenderPaintedLayerBlob.h
@@ +38,5 @@
> +  }
> +  WebRenderLayerManager* Manager()
> +  {
> +    return static_cast<WebRenderLayerManager*>(mManager);
> +  }

Don't need this Manager() function. Use WrManager() which already exists and does the same thing. related: bug 1360449.

@@ +52,5 @@
> +
> +  Layer* GetLayer() override { return this; }
> +  void RenderLayer(wr::DisplayListBuilder& aBuilder) override;
> +  RefPtr<ImageContainer> mImageContainer;
> +  RefPtr<ImageClient> mImageClient;

Do mImageContainer and mImageClient need to be public?

@@ +57,5 @@
> +
> +private:
> +  bool SetupExternalImages();
> +  bool UpdateImageClient();
> +  void CreateWebRenderDisplayList(wr::DisplayListBuilder& aBuilder);

Too much copypasta. These functions don't have implementations, so remove them.

::: gfx/thebes/gfxPrefs.h
@@ +689,4 @@
>  
>    // WARNING:
>    // Please make sure that you've added your new preference to the list above in alphabetical order.
>    // Please do not just append it to the end of the list.

Obey the warning ^

::: gfx/webrender_bindings/Moz2DImageRenderer.cpp
@@ +11,2 @@
>  #include "WebRenderTypes.h"
> +#include "gfxUtils.h"

Sort includes. gfxUtils.h goes at the top, and swap the order of the two others you added.

@@ +22,5 @@
> +  InMemoryStreamBuffer(const Range<const uint8_t> aBlob)
> +  {
> +    // we need to cast away the const because C++ doesn't
> +    // have a separate type of streambuf that can only
> +    // be read from

Thanks for the comment!

@@ +36,2 @@
>  {
> +  printf_stderr("Callback: %d %d\n", aSize.width, aSize.height);

Maybe comment this out for now?

@@ +68,5 @@
> +  gfx::Matrix baseTransform;
> +  gfx::InlineTranslator translator(dt, baseTransform);
> +  static int i = 0;
> +  char filename[40];
> +  sprintf(filename, "out%d.png", i++);

I'd move these three lines down to just before the WriteAsPNG and comment them all out together. Not a big deal though.

Comment 5

[Lee Salzman [:lsalzman]]

Comment on attachment 8862711 [details] [diff] [review]
Add back some removed code for in-memory recording surfaces

Bob Owen should really be reviewing this since it was ultimately his decision to remove it.

That said, this will bring back all the risks of OOMing, since stringbuf is infallible.

The BasicLayersImpl part should also probably remain removed as that was only there to paper over another bug that should have not required this.

There are also potential problems here with SourceSurfaceRecording not remembering its proper place in the stream that may come back to haunt us in certain situations. I had a potential patch for this in one of the bugs.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to Lee Salzman [:lsalzman] from comment #5)
> Comment on attachment 8862711 [details] [diff] [review]
> Add back some removed code for in-memory recording surfaces
> 
> Bob Owen should really be reviewing this since it was ultimately his
> decision to remove it.
> 
> That said, this will bring back all the risks of OOMing, since stringbuf is
> infallible.

Do you know what situations caused OOMs?

Comment 7

[Lee Salzman [:lsalzman]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> (In reply to Lee Salzman [:lsalzman] from comment #5)
> > Comment on attachment 8862711 [details] [diff] [review]
> > Add back some removed code for in-memory recording surfaces
> > 
> > Bob Owen should really be reviewing this since it was ultimately his
> > decision to remove it.
> > 
> > That said, this will bring back all the risks of OOMing, since stringbuf is
> > infallible.
> 
> Do you know what situations caused OOMs?

Since stringbuf uses infallible allocations, any recording will risk OOMs due to large buffers getting allocated during serialization. We had a ton of problems with this when Tobias had used DrawEventRecorderMemory for printing. It only took buffers on the order of a several MBs to start triggering those.

Comment 8

[Bob Owen (:bobowen)]

Comment on attachment 8862711 [details] [diff] [review]
Add back some removed code for in-memory recording surfaces

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

In addition to what Lee said about the infallible side of things.
I think that things are compounded by the fact stringbuf allocates continuous memory.

I can't remember who mentioned it originally, but because of the way jemalloc handles things once we start requesting infallible buffers of > 2MB, then things are going to OOM eventually.

With the way stringbuf grows this happens quite a lot, so can trigger the issue quite quickly.

Actually just found the comment in bug 1279699 comment 18 (it was gcp), but I suspect glandium would be able to shed most light on the issue.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Bob Owen (:bobowen) from comment #8)
> Comment on attachment 8862711 [details] [diff] [review]
> Add back some removed code for in-memory recording surfaces
> 
> Review of attachment 8862711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In addition to what Lee said about the infallible side of things.
> I think that things are compounded by the fact stringbuf allocates
> continuous memory.

I don't intend to ship using the current stringbuf serializer and will be replacing it before shipping. I'd like to get this in now so that other recording surface stuff can be worked on and tested.

Comment 10

[Bob Owen (:bobowen)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> (In reply to Bob Owen (:bobowen) from comment #8)
> > Comment on attachment 8862711 [details] [diff] [review]
> > Add back some removed code for in-memory recording surfaces
> > 
> > Review of attachment 8862711 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > In addition to what Lee said about the infallible side of things.
> > I think that things are compounded by the fact stringbuf allocates
> > continuous memory.
> 
> I don't intend to ship using the current stringbuf serializer and will be
> replacing it before shipping. I'd like to get this in now so that other
> recording surface stuff can be worked on and tested.

Fair enough, but can we just include the parts that are needed then and add a comment to DrawEventRecorderMemory explaining that it must not be used in shipped code in its current state.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Looks ok mostly, but enough things that I'd like to see a new version. Also
> add a real commit message.

I'm sorry you had to suffer through such terrible code.

> 
> ::: gfx/layers/moz.build
> @@ +407,5 @@
> >      'wr/WebRenderLayer.cpp',
> >      'wr/WebRenderLayerManager.cpp',
> >      'wr/WebRenderLayersLogging.cpp',
> >      'wr/WebRenderPaintedLayer.cpp',
> > +    'wr/WebRenderPaintedLayerBlob.cpp',
> 
> Should add the .h file to moz.build as well

Why? WebRenderPaintedLayer.h isn't included and I don't think we want to export.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add WebRenderPaintedLayerBlob

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add back some removed code for in-memory recording surfaces

Comment 14

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

Comment on attachment 8862946 [details] [diff] [review]
Add WebRenderPaintedLayerBlob

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

::: gfx/layers/wr/WebRenderPaintedLayerBlob.cpp
@@ +12,5 @@
> +#include "mozilla/layers/StackingContextHelper.h"
> +#include "mozilla/layers/WebRenderBridgeChild.h"
> +#include "mozilla/layers/UpdateImageHelper.h"
> +#include "mozilla/webrender/WebRenderTypes.h"
> +#include "mozilla/gfx/DrawEventRecorder.h"

Move this mozilla/gfx include up to after ArrayUtils.h

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 8862949 [details] [diff] [review]
Add back some removed code for in-memory recording surfaces

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

::: gfx/2d/DrawTargetRecording.h
@@ +330,5 @@
>    RefPtr<DrawEventRecorderPrivate> mRecorder;
>    RefPtr<DrawTarget> mFinalDT;
>  };
>  
> +class SourceSurfaceRecording : public SourceSurface

I think this class was moved to the header for the changes in BasicLayersImpl.cpp, so I don't think we need this change.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/de635d62079b
Add back some recording helpers. r=bobowen
https://hg.mozilla.org/projects/graphics/rev/8f1d4b0a03e4
Add WebRenderPaintedLayerBlob for painting with BlobImages. r=kats

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/de635d62079b
https://hg.mozilla.org/mozilla-central/rev/8f1d4b0a03e4