Closed Bug 1257101 Opened 8 years ago Closed 8 years ago

imgFrame::IsImageComplete says whether we've had pixels decoded to the whole image, but it's used to check if the frame is finished decoding. these are different things when the image has more than one progress pass.

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

()

Details

Attachments

(1 file)

      No description provided.
http://lcamtuf.coredump.cx/ffjpeg loads the same jpeg many times and draws it to a canvas and compares the output to see if any differ. Saving the page locally (and modifying it as necessary, running from a local web server) doesn't seem to reproduce the problem. Maybe the server coredump.cx is chunking the image differently or delivering it more slowly?
Attached patch patchSplinter Review
Assignee: nobody → tnikkel
Attachment #8731088 - Flags: review?(seth)
Oh, and since this affects the code that records image decode time for telemetry we should expect a change in those numbers.
Comment on attachment 8731088 [details] [diff] [review]
patch

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

Looks good! Some tweaks below, though.

::: image/imgFrame.cpp
@@ +161,5 @@
>  {
>  #ifdef DEBUG
>    MonitorAutoLock lock(mMonitor);
>    MOZ_ASSERT(mAborted || IsImageCompleteInternal());
> +  MOZ_ASSERT(mAborted || mFinished);

Rename |IsImageCompleteInternal()| to something like |AreAllPixelsWritten()|. That name makes much more sense, and it'll make the two remaining uses (this one and the one in Draw()) read much better.

@@ +323,5 @@
>  
>    // If we reach this point, we should regard ourselves as complete.
>    mDecoded = GetRect();
> +  mFinished = true;
> +  // Take the monitor just for IsImageCompleteInternal

You probably don't need this comment at all, either, given the change below...

@@ +324,5 @@
>    // If we reach this point, we should regard ourselves as complete.
>    mDecoded = GetRect();
> +  mFinished = true;
> +  // Take the monitor just for IsImageCompleteInternal
> +  MonitorAutoLock lock(mMonitor);

Put this in an #ifdef DEBUG; we don't want to take the monitor in release builds.

@@ +651,5 @@
>    mTimeout = aRawTimeout;
>    mBlendMethod = aBlendMethod;
>    ImageUpdatedInternal(GetRect());
> +  mFinished = true;
> +  // The image is now complete, wake up anyone who's waiting.

Please add a blank line above this one.

@@ +839,5 @@
>  
>    MOZ_ASSERT(mLockCount > 1 || IsImageCompleteInternal() || mAborted,
>               "Should have marked complete or aborted before unlocking");
> +  MOZ_ASSERT(mLockCount > 1 || mFinished || mAborted,
> +             "Should have Finish()'d or aborted before unlocking");

Just get rid of the old assertion on the line above. mFinished is playing precisely the same role that the IsImageComplete() test did before. Keep in mind that calling Finish(), where mFinished is set, guarantees that IsImageComplete() would return true.
Attachment #8731088 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/c75b2b195f28
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This page sometimes still shows 2 variants with just downloaded FireFox 48 (Windows 64 Bit, Vista):
http://lcamtuf.coredump.cx/chjpeg/index2a.html
So perhaps bug is not completely fixed?
Really completely fixed? See/test URL in my last comment...
Flags: needinfo?(tnikkel)
(In reply to Martin.github.Maurer from comment #10)
> Really completely fixed? See/test URL in my last comment...

That would be a different testcase. This bug/patch isn't intended to fix every bug, just the testcase in comment 1. Please file a new bug with your testcase and cc me.
Flags: needinfo?(tnikkel)
Flags: needinfo?(Martin.github.Maurer)
New bug filed, see here
https://bugzilla.mozilla.org/show_bug.cgi?id=1294822
Flags: needinfo?(Martin.github.Maurer)
You need to log in before you can comment on or make changes to this bug.