Closed Bug 888499 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics: ImageLib, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: joe, Assigned: joe)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Mark frames dirty (obsolete) — Splinter Review
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.
Attachment #769210 - Flags: review?(seth)
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.
Attachment #769210 - Flags: review?(seth) → review+
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 :)
Attachment #769210 - Flags: review+ → review-
This adds a Mutex to ensure a write barrier.
Attachment #769210 - Attachment is obsolete: true
Attachment #770360 - Flags: review?(seth)
Comment on attachment 770360 [details] [diff] [review]
mark frames dirty v2

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

Sweet!
Attachment #770360 - Flags: review?(seth) → review+
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.)
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/6bc751e34867
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 950731
You need to log in before you can comment on or make changes to this bug.