888499 - Mark frames as dirty when ImageUpdated is called on them, and then clear that dirty bit when drawing

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

Attachment: Mark frames dirty

imgFrame::ImageUpdated used to explicitly MarkDirty the frame's surfaces, but that was fixed in bug 855221 since ImageUpdated can be called on any frame. We get lucky in that (currently) FrameBlender locks and unlocks frames when blending (which implicitly flushes/markdirty), but that's changing in bug 717872. That means we need to be more explicit about things.

This does the MarkImageDataDirty just before drawing, but I'm willing to listen on where a better place might be for that. The important thing is that it needs to happen on the main thread.

Comment 1

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

Comment on attachment 769210 [details] [diff] [review]
Mark frames dirty

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

::: image/src/imgFrame.cpp
@@ +498,5 @@
>    // you, gif decoder)
>    nsIntRect boundsRect(mOffset, mSize);
>    mDecoded.IntersectRect(mDecoded, boundsRect);
>  
> +  mDirty = true;

This is not safe because you cannot be sure that the mDirty will not get set to true before the other stuff in this method happens, from the perspective of another thread. It will almost certainly end up working anyway, but that doesn't mean it's a good idea. This is undefined behavior in C++11.

Also, if ImageUpdated can be called multiple times on another thread before we end up calling MarkImageDataDirty on the main thread, the main thread may observe an inconsistent state when it DOES call MarkImageDataDirty because we may be part way through a second update to mDecoded.

In my opinion you should just use appropriately scoped locking unless you measure there to be an actual performance problem with doing so.

Comment 2

[Joe Drew (not getting mail)]

Comment on attachment 769210 [details] [diff] [review]
Mark frames dirty

Heh, that reads a lot more like an r-, so I'm going to pretend that's what you gave me :)

Comment 3

[Joe Drew (not getting mail)]

Attachment: mark frames dirty v2

This adds a Mutex to ensure a write barrier.

Comment 4

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

Comment on attachment 770360 [details] [diff] [review]
mark frames dirty v2

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

Sweet!

Comment 5

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

Comment on attachment 770360 [details] [diff] [review]
mark frames dirty v2

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

::: image/src/RasterImage.cpp
@@ +2695,5 @@
>                        framerect.x);
>  
> +  if (frame->GetIsDirty()) {
> +    frame->MarkImageDataDirty();
> +  }

Ack, sorry, realized this right after r+'ing. You probably want to fold the GetIsDirty check into MarkImageDataDirty itself, to avoid taking the lock twice. I don't see a correctness problem here since only MarkImageDataDirty can set mDirty to false once it's true, so this will work right as long as only one thread calls MarkImageDataDirty, but for perf reasons it'd be nice to fix this. (And it will have the bonus of being safe even if MarkImageDataDirty is eventually called from multiple threads.)

Comment 6

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

(In reply to Joe Drew (:JOEDREW! \o/) from comment #2)
> Comment on attachment 769210 [details] [diff] [review]
> Mark frames dirty
> 
> Heh, that reads a lot more like an r-, so I'm going to pretend that's what
> you gave me :)

Yeah, you're right. =) I think the r+ for the new patch is OK, though, in that I don't think it will require re-review after that change.

Comment 7

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #5)
> Ack, sorry, realized this right after r+'ing. You probably want to fold the
> GetIsDirty check into MarkImageDataDirty itself, to avoid taking the lock
> twice.

Sure.

> (And it will have the bonus of being safe even if
> MarkImageDataDirty is eventually called from multiple threads.)

This *should* never be the case (since it's doing Thebes stuff, which is inherently main-thread only), but it doesn't hurt.

Comment 8

[Joe Drew (not getting mail)]

imgFrame::ApplyDirtToSurfaces()

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bc751e34867

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6bc751e34867