962670 - Store decoded images in VolatileBuffers

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Michael Wu [:mwu]]

VolatileBuffers let us leave it to the OS to discard images.

Comment 1

[Michael Wu [:mwu]]

Attachment: Add gfxLockedImageSurface

This is a gfxImageSurface that locks a VolatileBuffer while it exists. When the surface is destroyed, the VolatileBuffer is unlocked.

Comment 2

[Michael Wu [:mwu]]

Attachment: WIP: Make imgFrame store images in VolatileBuffers

This is pretty close - I think I just have to adjust the memory reporting and make sure tests pass.

Comment 3

[Michael Wu [:mwu]]

Attachment: Add gfxLockedImageSurface, v2

This makes gfxQuartzImageSurfaces compatible with gfxLockedImageSurfaces.

Comment 4

[Michael Wu [:mwu]]

Attachment: WIP: Make imgFrame store images in VolatileBuffers, v2

This is actually pretty close to done, but it fails some reftests still..

Comment 5

[Michael Wu [:mwu]]

Attachment: Add gfxLockedImageSurface, v3

Updated for some API changes in VolatileBuffers

Comment 6

[Michael Wu [:mwu]]

Attachment: WIP: Make imgFrame store images in VolatileBuffers, v3

Updated for VolatileBuffer API changes

Comment 7

[Michael Wu [:mwu]]

Attachment: [1/3] Add gfxLockedImageSurface, v4

Fix build on debug builds.

Comment 8

[Michael Wu [:mwu]]

Attachment: Make imgFrame store images in VolatileBuffers, v4

Comment 9

[Michael Wu [:mwu]]

Attachment: Make imgFrame store images in VolatileBuffers, v4 (wrong patch)

Comment 10

[Michael Wu [:mwu]]

Attachment: [3/3] Avoid unnecessary invalidation due to changing ImageContainers

Since a new ImageContainer is generated whenever GetImageContainer is called on RasterImage, the current logic LayerTreeInvalidation will keep thinking the image has changed. (and cause certain reftests to time out) Here is one simple fix, probably wrong.

Comment 11

[Michael Wu [:mwu]]

Attachment: [2/3] Make imgFrame store images in VolatileBuffers, v5

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 8368859 [details] [diff] [review]
[2/3] Make imgFrame store images in VolatileBuffers, v5

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

This makes imgFrame primarily hold on to image data via VolatileBuffers rather than gfxImageSurfaces. Whenever a surface is necessary, one is generated to wrap the VolatileBuffer as necessary. If the buffer was purged, RasterImage will start decoding again. ImageContainers are also generated as necessary, in order to avoid locking the buffer. This confuses the layer tree invalidation a bit. I have a few ideas on how to avoid that.

::: image/src/RasterImage.cpp
@@ +2119,5 @@
> +  if (GetNumFrames() > 0) {
> +    imgFrame *curframe = mFrameBlender.RawGetFrame(GetNumFrames() - 1);
> +    curframe->UnlockImageData();
> +  }
> +

The unlock is moved after Finish() since Finish tries to optimize the image. Unlocking the image here may remove access to the image surface.

::: image/src/imgFrame.cpp
@@ +837,5 @@
>  #endif
>  #ifdef XP_MACOSX
>    if (mQuartzSurface && aLocation == gfxMemoryLocation::IN_PROCESS_HEAP) {
> +    n += aMallocSizeOf(mQuartzSurface);
> +  }

This calculation seemed very wrong to me, so I fixed it while adding the size calculation for volatile buffers.

@@ +855,5 @@
> +    n += aMallocSizeOf(mVBuf);
> +    if (mVBuf->OnHeap()) {
> +      n += mVBuf->SizeOf(aMallocSizeOf);
> +    }
> +  }

This only counts the memory used by the surface if we're allocated on heap. We'll probably need to add more memory instrumentation for things stored in actual volatile buffers.

Comment 13

[Michael Wu [:mwu]]

Attachment: [3/3] Avoid unnecessary invalidation due to changing ImageContainers, v2

I think this fix should be right.

Comment 14

[Michael Wu [:mwu]]

Attachment: [2/3] Make imgFrame store images in VolatileBuffers, v6

This uses GetSurface rather than LockImageSurface in the RasterImage::Draw path so we don't accidentally unoptimize images.

Comment 15

[Michael Wu [:mwu]]

Attachment: [2/3] Make imgFrame store images in VolatileBuffers, v7

Forgot to account for single color images in the last patch.

Comment 16

[Michael Wu [:mwu]]

I also have patches to make scaled images discardable and to make 16bit (RGB565) optimized images store in volatile buffers.

Comment 17

[Michael Wu [:mwu]]

Attachment: [1/3] Add gfxLockedImageSurface, v5

Updated for VolatileBuffer API changes.

Comment 18

[Michael Wu [:mwu]]

Attachment: [2/3] Make imgFrame store images in VolatileBuffers, v8

Update for VolatileBuffer API changes.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8372659 [details] [diff] [review]
[2/3] Make imgFrame store images in VolatileBuffers, v8

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

::: image/src/RasterImage.cpp
@@ +925,3 @@
>      frame->GetSurface(getter_AddRefs(framesurf));
> +    if (!framesurf && !frame->IsSinglePixel()) {
> +      // No reason to be optimized away here - the OS threw out the data

If there's a way you can assert that this is indeed the case, you should assert it. Murphy's Law suggests that down the road a bug will make us hit this pathway for some other reason.

@@ +928,5 @@
> +      if (!(aFlags & FLAG_SYNC_DECODE))
> +        return nullptr;
> +
> +      ForceDiscard();
> +      return GetFrame(aWhichFrame, aFlags);

You probably need to call CanForciblyDiscardAndRedecode() to verify it's safe to call ForceDiscard().

@@ +956,5 @@
>  
>    nsRefPtr<gfxASurface> imageSurface = GetFrame(FRAME_CURRENT, FLAG_NONE);
> +  if (!imageSurface) {
> +    // The OS threw out some or all of our buffer. Start decoding again.
> +    ForceDiscard();

Could also use an assertion and a call to CanForciblyDiscardAndRedecode() here.

@@ +1009,5 @@
>    *_retval = mImageContainer;
>    NS_ADDREF(*_retval);
> +  if (CanForciblyDiscardAndRedecode()) {
> +    mImageContainer = nullptr;
> +  }

Why are you calling CanForciblyDiscardAndRedecode() when you are neither discarding nor redecoding? (Well, you're not doing it here, at least. GetCurrentImage() might do it.)

@@ +2682,5 @@
> +  if (!frame->IsSinglePixel()) {
> +    frame->GetSurface(getter_AddRefs(surf));
> +    if (!surf) {
> +      // The OS threw out some or all of our buffer. Start decoding again.
> +      ForceDiscard();

Same comment as the two other ForceDiscard calls.

Comment 20

[Michael Wu [:mwu]]

Comment on attachment 8372659 [details] [diff] [review]
[2/3] Make imgFrame store images in VolatileBuffers, v8

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

::: image/src/RasterImage.cpp
@@ +928,5 @@
> +      if (!(aFlags & FLAG_SYNC_DECODE))
> +        return nullptr;
> +
> +      ForceDiscard();
> +      return GetFrame(aWhichFrame, aFlags);

The check is done by RasterImage::DecodingComplete. We only allow GetSurface to return null if

1. It's an indexed image. But we shouldn't hit the RasterImage::Draw path or this code in that case..
2. It's a single color/pixel image, which we check for here.
3. It's an image whose data got discarded by the OS. The data can only be discarded if there's no gfxLockedImageSurfaces holding on to it. We only allow imgFrame::mImageSurface to be set to null if we've previously determined that the image can forcibly discarded and redecoded. SetDiscardable is called on imgFrame if that is the case. mImageSurface can also be set to null by imgFrame::Optimize(), but in that case it is replaced by mOptSurface, so GetSurface would not return null.

@@ +1009,5 @@
>    *_retval = mImageContainer;
>    NS_ADDREF(*_retval);
> +  if (CanForciblyDiscardAndRedecode()) {
> +    mImageContainer = nullptr;
> +  }

If we can't discard and recode, then that means the imgFrame didn't get SetDiscardable() called on it and we don't need to be careful about releasing the image data back to the OS (by releasing all references to the gfxLockedImageSurface). More specifically, it lets us avoid reallocating the ImageContainer in cases where the image isn't discardable by the OS.

::: image/src/imgFrame.h
@@ +123,5 @@
> +#else
> +      return sur;
> +#endif
> +    }
> +    return nullptr;

This is probably a pretty good place to assert, since we should never return nullptr unless it's a single pixel optimized image, indexed image, or an OS discarded image.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

I'll try to get to this review today.

Comment 22

[Michael Wu [:mwu]]

Attachment: [2/3] Make imgFrame store images in VolatileBuffers, v9

Unbitrotted, updated for the latest VolatileBuffer API update, and assertion added to ThebesSurface().

Comment 23

[Jeff Muizelaar [:jrmuizel]]

mwu,

Do you have any idea what the overhead is for VolatileBuffer::Lock() (in the case when the memory has not been purged) on the different platforms in number of cycles? The windows version looks like it could be heavy weight. Are memory maps being changed?

Comment 24

[Michael Wu [:mwu]]

Hmm, I don't know what the overhead is. Looking at the ashmem implementation, it seems to run in constant time (in our case) and the implementation is relatively straightforward. On OSX/mach, the implementation has to mark every page for LRU purposes. The mapping is not changed on any platform after the VolatileBuffer is generated - the OS always uses the same address range.

Comment 25

[Jeff Muizelaar [:jrmuizel]]

(In reply to Michael Wu [:mwu] from comment #24)
> Hmm, I don't know what the overhead is. Looking at the ashmem
> implementation, it seems to run in constant time (in our case) and the
> implementation is relatively straightforward. On OSX/mach, the
> implementation has to mark every page for LRU purposes. The mapping is not
> changed on any platform after the VolatileBuffer is generated - the OS
> always uses the same address range.

Do you mind measuring so we have some idea? i.e. just run with this patch and TimeStamp around the operation.

Comment 26

[Michael Wu [:mwu]]

BTW these patches don't anything on Windows since we start with a gfxWindowsSurface rather than VolatileBuffer + gfxLockedImageSurface. I do have a patch which removes the use of gfxWindowsSurface in imgFrame but I have no idea if that's a good idea or not.

Comment 27

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8369086 [details] [diff] [review]
[3/3] Avoid unnecessary invalidation due to changing ImageContainers, v2

I don't think I'm the right person to review this. Including the imgIContainer in the ImageContainer doesn't feel great to me.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Michael Wu [:mwu] from comment #10)
> Since a new ImageContainer is generated whenever GetImageContainer is called
> on RasterImage,

But it isn't! The image container is cached in RasterImage::mImageContainer.

Comment 29

[Michael Wu [:mwu]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> (In reply to Michael Wu [:mwu] from comment #10)
> > Since a new ImageContainer is generated whenever GetImageContainer is called
> > on RasterImage,
> 
> But it isn't! The image container is cached in RasterImage::mImageContainer.

That's what the 2nd patch in this series does - it stops caching it so we don't pin the image down when we're not using it.

Comment 30

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8372658 [details] [diff] [review]
[1/3] Add gfxLockedImageSurface, v5

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

I feel like it might be better to use SetData() api to preserve the lifetime of the the VolatileBufferPtr. That way we don't have to do the weird gymnastics in gfxQuartzImageSurface. This will also make it easier to port Moz2D because we won't need a dependency on the VolatileBuffer stuff.

Comment 31

[Seth Fowler [:seth] [:s2h]]

(In reply to Michael Wu [:mwu] from comment #20)
> The check is done by RasterImage::DecodingComplete. We only allow GetSurface
> to return null if

This reasoning is fine, but right now you've documented in a comment what it seems to me you could just as well document *and enforce* with an assertion. Complex invariants will not remain true over time unless we have some sort of check to help keep them in place.

> If we can't discard and recode, then that means the imgFrame didn't get
> SetDiscardable() called on it and we don't need to be careful about
> releasing the image data back to the OS

Thanks for the explanation! That should be documented in a comment. Beyond that, part of the reason that wasn't clear during the initial review is that you use a different test when you decide whether to call SetDiscardable initially. Why aren't they the same?

> This is probably a pretty good place to assert, since we should never return
> nullptr unless it's a single pixel optimized image, indexed image, or an OS
> discarded image.

I'm all for placing assertions anywhere you think they'd be useful.

Comment 32

[Michael Wu [:mwu]]

Attachment: [1/2] Add gfxLockedImageSurface, v6

This is kinda horrible, but it's the most efficient way to work with SetData. The alternative is to allocate a VolatileBufferPtr, which seems like a waste.. (I do have a patch for that if that's preferred)

Comment 33

[Michael Wu [:mwu]]

Attachment: [2/2] Make imgFrame store images in VolatileBuffers, v10

Ok, this takes a different approach. When we have a discardable image, we use a weak pointer to hold onto the ImageContainer. Requested comments also added.

Haven't had a chance to measure the time taken by locking volatile buffers yet.

Comment 34

[Mike Hommey [:glandium]]

Comment on attachment 8376878 [details] [diff] [review]
[1/2] Add gfxLockedImageSurface, v6

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

Not sure what i'm supposed to review here.

Comment 35

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8376878 [details] [diff] [review]
[1/2] Add gfxLockedImageSurface, v6

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

::: gfx/thebes/gfxLockedImageSurface.h
@@ +11,5 @@
> +/*
> + * This is just a gfxImageSurface which will unlock the buffer on destruction
> + */
> +
> +class gfxLockedImageSurface : public gfxImageSurface

I think you can just turn this into a helper class. You shouldn't need to inherit from gfxImageSurface. Just have the users of this set SetData() on the surfaces they are using.

Comment 36

[Michael Wu [:mwu]]

Attachment: [1/2] Add gfxLockedImageSurface, v7

I was also thinking inheriting from gfxImageSurface is pointless now.

Comment 37

[Michael Wu [:mwu]]

Comment on attachment 8381152 [details] [diff] [review]
[1/2] Add gfxLockedImageSurface, v7

Requesting review from glandium for the VolatileBuffer api change.

Comment 38

[Michael Wu [:mwu]]

Attachment: [2/2] Make imgFrame store images in VolatileBuffers, v11

Adjusted patch due to changes in gfxLockedImageSurface.

Comment 39

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8381154 [details] [diff] [review]
[2/2] Make imgFrame store images in VolatileBuffers, v11

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

Thanks for adding those comments. They make your intent much clearer.

::: image/src/imgFrame.h
@@ +195,5 @@
>  
>    /** Indicates how many readers currently have locked this frame */
>    int32_t mLockCount;
>  
> +  mozilla::RefPtr<mozilla::VolatileBuffer> mVBuf;

This seems like a pretty classic case for |mutable| but I don't think we need to worry too much about usage of const imgFrame references right now so I'm happy either way.

Comment 40

[Mike Hommey [:glandium]]

Comment on attachment 8381152 [details] [diff] [review]
[1/2] Add gfxLockedImageSurface, v7

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

It would be better to avoid exposing those guts. How about this:

::: gfx/thebes/gfxLockedImageSurface.cpp
@@ +14,5 @@
> +VolatileBufferRelease(void *buf)
> +{
> +    VolatileBuffer *vbuf = static_cast<VolatileBuffer*>(buf);
> +    vbuf->Unlock();
> +    vbuf->Release();

VolatileBufferPtr *vbuf = static_cast<VolatileBufferPtr*>(buf);
delete vbuf;

@@ +35,5 @@
> +
> +    void *buf;
> +    vbuf->AddRef();
> +    vbuf->Lock(&buf);
> +    img->SetData(&kVolatileBuffer, vbuf, VolatileBufferRelease);

VolatileBufferPtr *buf = new VolatileBufferPtr(vbuf);
img->SetData(&kVolatileBuffer, buf, VolatileBufferRelease);

(I wonder if it wouldn't be possible to use operator delete directly here instead of VolatileBufferRelease)

Comment 41

[Michael Wu [:mwu]]

Attachment: Make imgFrame store images in VolatileBuffers, v12

I combined the patches into one. gfxLockedImageSurface is now in imgFrame.cpp, and will probably be removed in the future with some other patches I'm working on.

The time taken to lock a volatile buffer generally appears to be around 5-15 microseconds on a relatively recent Mac Mini running OSX 10.9 with a 2.3ghz i7. Occasionally goes over 20 microseconds, but it's very rare. Also occasionally takes around 200 nanoseconds, which I assume is the case when we're already locked and are taking the fast path that just increments a counter.

Comment 42

[Michael Wu [:mwu]]

Just tested on a Flame device (1.2ghz dual core - MSM8610). Seems to take around 15-20 microseconds with the occasional jump to 40-50 microseconds. Fast path takes about 1 microsecond.

Comment 43

[Michael Wu [:mwu]]

Comment on attachment 8384357 [details] [diff] [review]
Make imgFrame store images in VolatileBuffers, v12

Seth, do you mind reviewing the locked image surface stuff that I've moved into RasterImage.cpp?

Comment 44

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8384357 [details] [diff] [review]
Make imgFrame store images in VolatileBuffers, v12

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

::: image/src/imgFrame.cpp
@@ +626,5 @@
> +  if (!mImageSurface) {
> +    if (mVBuf) {
> +      VolatileBufferPtr<uint8_t> ref(mVBuf);
> +      if (ref.WasBufferPurged())
> +        return NS_ERROR_FAILURE;

LockedImageSurface::CreateSurface does this same check. CreateSurface should either take a VolatileBufferPtr or this check should be omitted.

Comment 45

[Michael Wu [:mwu]]

Comment on attachment 8384357 [details] [diff] [review]
Make imgFrame store images in VolatileBuffers, v12

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

::: image/src/imgFrame.cpp
@@ +626,5 @@
> +  if (!mImageSurface) {
> +    if (mVBuf) {
> +      VolatileBufferPtr<uint8_t> ref(mVBuf);
> +      if (ref.WasBufferPurged())
> +        return NS_ERROR_FAILURE;

The check in CreateSurface is only an assertion and wouldn't happen on normal builds. The CreateSurface caller needs to ensure that their image data is still available before wrapping it with a gfxImageSurface.

Comment 46

[Jeff Muizelaar [:jrmuizel]]

(In reply to Michael Wu [:mwu] from comment #45)
> Comment on attachment 8384357 [details] [diff] [review]
> Make imgFrame store images in VolatileBuffers, v12
> 
> Review of attachment 8384357 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/imgFrame.cpp
> @@ +626,5 @@
> > +  if (!mImageSurface) {
> > +    if (mVBuf) {
> > +      VolatileBufferPtr<uint8_t> ref(mVBuf);
> > +      if (ref.WasBufferPurged())
> > +        return NS_ERROR_FAILURE;
> 
> The check in CreateSurface is only an assertion and wouldn't happen on
> normal builds. The CreateSurface caller needs to ensure that their image
> data is still available before wrapping it with a gfxImageSurface.

Wouldn't it be better for CreateSurface to just take a VolatileBufferPtr then?

Comment 47

[Michael Wu [:mwu]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #46)
> (In reply to Michael Wu [:mwu] from comment #45)
> > Comment on attachment 8384357 [details] [diff] [review]
> > Make imgFrame store images in VolatileBuffers, v12
> > 
> > Review of attachment 8384357 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: image/src/imgFrame.cpp
> > @@ +626,5 @@
> > > +  if (!mImageSurface) {
> > > +    if (mVBuf) {
> > > +      VolatileBufferPtr<uint8_t> ref(mVBuf);
> > > +      if (ref.WasBufferPurged())
> > > +        return NS_ERROR_FAILURE;
> > 
> > The check in CreateSurface is only an assertion and wouldn't happen on
> > normal builds. The CreateSurface caller needs to ensure that their image
> > data is still available before wrapping it with a gfxImageSurface.
> 
> Wouldn't it be better for CreateSurface to just take a VolatileBufferPtr
> then?

VolatileBufferPtrs are normally allocated on stack. It's a bit unusual here that we're allocating one here on heap - basically no other code will need to do that.

Comment 48

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/b2g-inbound/rev/965fe5b195ab

Comment 49

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/088af92c1a44 for making everything leak a gfxASurface (https://tbpl.mozilla.org/php/getParsedLog.php?id=35768197&tree=B2g-Inbound etc.)

Comment 50

[Michael Wu [:mwu]]

Thanks philor. Looks like I leaked a gfxImageSurface on a OSX specific path in ThebesSurface().

Comment 51

[Michael Wu [:mwu]]

Let's try again. I had to modify the gfxQuartzImageSurface path in imgFrame::ThebesSurface().

https://hg.mozilla.org/integration/b2g-inbound/rev/2429468e82dd

Comment 52

[Jeff Muizelaar [:jrmuizel]]

Could this cause bug 981082?

Comment 53

[Michael Wu [:mwu]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #52)
> Could this cause bug 981082?

Probably not - it's not done landing yet.

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2429468e82dd

Comment 55

[Michael Wu [:mwu]]

Looks like this regressed SVG-ASAP by about 10% or so. I think we can reduce the number of allocations to speed things back up.

Comment 56

[Joe Cheng [:jcheng] (please needinfo)]

Hi Ying Xu, heard that you will be doing uplifts to 1.3T branch. After you completed the uplift, can you please set status-b2g-v1.3T to fixed? please let us know if you have problems with it. thanks

Comment 57

[ying.xu]

Hi,mwu

Could you please rebase your patch on branch v1.3t?
There are some conflicts for the current patch.

Comment 58

[Michael Wu [:mwu]]

I'll check this patch in myself. It depends on a few other patches in order to apply properly.

Comment 59

[Michael Wu [:mwu]]

I uplifted these bugs to port this patch:

Bug 958758
Bug 890743

Requesting blocking on them.

Comment 60

[Michael Wu [:mwu]]

er, I accidentally unset that blocking flag

Comment 61

[Michael Wu [:mwu]]

http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/a390b3e7b6c3