Last Comment Bug 715240 - DECODE_ON_DRAW_LATENCY is wrong
: DECODE_ON_DRAW_LATENCY is wrong
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Jeff Muizelaar [:jrmuizel]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-04 11:33 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-01-05 08:45 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Only start tracking the time when we have all of the source data (696 bytes, patch)
2012-01-04 11:34 PST, Jeff Muizelaar [:jrmuizel]
joe: review+
justin.lebar+bug: feedback+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-01-04 11:33:12 PST

    
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-01-04 11:34:16 PST
Created attachment 585825 [details] [diff] [review]
Only start tracking the time when we have all of the source data
Comment 2 Justin Lebar (not reading bugmail) 2012-01-04 11:38:37 PST
It's wrong in two ways.

First, the check

>  if (!mDecoded) {
>      mDrawStartTime = TimeStamp::Now();
>  }

should be

>  if (!mDecoded && mHasSourceData)

because both predicates together mean "discarded".

Also, the IMAGE_DECODE_ON_DRAW_LATENCY histogram is not being frobbed often enough.  When I switch to a tab containing 45 large, discarded images, the histogram is frobbed only twice.
Comment 3 Justin Lebar (not reading bugmail) 2012-01-04 11:42:56 PST
Also, is Telemetry::IMAGE_DECODE_TIME supposed to track the time it takes to re-decode an image?  It doesn't appear to at the moment.
Comment 4 Justin Lebar (not reading bugmail) 2012-01-04 11:52:16 PST
> Also, the IMAGE_DECODE_ON_DRAW_LATENCY histogram is not being frobbed often enough.  When I switch 
> to a tab containing 45 large, discarded images, the histogram is frobbed only twice.

Oh.  The images aren't decoded until they appear onscreen.
Comment 5 Justin Lebar (not reading bugmail) 2012-01-04 12:09:52 PST
Comment on attachment 585825 [details] [diff] [review]
Only start tracking the time when we have all of the source data

This looks right to me, but I'd like joe to have a look.
Comment 6 Joe Drew (not getting mail) 2012-01-04 13:52:02 PST
Comment on attachment 585825 [details] [diff] [review]
Only start tracking the time when we have all of the source data

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

::: image/src/RasterImage.cpp
@@ +2578,5 @@
>      mFrameDecodeFlags = DECODE_FLAGS_DEFAULT;
>    }
>  
> +  // XXX: we use !mDecoded && mHasSourceData to mean
> +  // discarded. I'm not sure if this is true or not.

This is correct. Fix up the comment.
Comment 7 Marco Bonardo [::mak] 2012-01-05 08:45:06 PST
https://hg.mozilla.org/mozilla-central/rev/a5fa5e73d901

Note You need to log in before you can comment on or make changes to this bug.