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)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: tnikkel, Assigned: tnikkel)
References
()
Details
Attachments
(1 file)
13.24 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
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?
Assignee | ||
Comment 2•8 years ago
|
||
Assignee: nobody → tnikkel
Attachment #8731088 -
Flags: review?(seth)
Assignee | ||
Comment 3•8 years ago
|
||
Oh, and since this affects the code that records image decode time for telemetry we should expect a change in those numbers.
Comment 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9868f86ab00
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c75b2b195f28
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 9•8 years ago
|
||
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?
Comment 10•8 years ago
|
||
Really completely fixed? See/test URL in my last comment...
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 11•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(Martin.github.Maurer)
Comment 12•8 years ago
|
||
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.
Description
•