Last Comment Bug 580531 - Expose framerate/statistics of <video> playback
: Expose framerate/statistics of <video> playback
Status: VERIFIED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- enhancement with 2 votes (vote)
: mozilla5
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on: 646329 post2.0 654550
Blocks: 592833
  Show dependency treegraph
 
Reported: 2010-07-20 23:14 PDT by cajbir (:cajbir)
Modified: 2012-05-02 22:10 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
HTML file using mozFrameCount to display framerate (1.33 KB, text/html)
2010-07-20 23:42 PDT, cajbir (:cajbir)
no flags Details
Implement mozFrameCount on video elements (4.45 KB, patch)
2010-07-20 23:44 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Implements playback statistics on video and media elements (23.76 KB, patch)
2010-08-12 22:28 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
HTML file example to display dropped frames, download rate and other stats (1.83 KB, text/html)
2010-08-12 22:29 PDT, cajbir (:cajbir)
no flags Details
HTML file example to display dropped frames, download rate and other stats (1.83 KB, text/html)
2010-08-23 22:05 PDT, cajbir (:cajbir)
no flags Details
Implements playback statistics on video and media elements (24.24 KB, patch)
2010-08-23 22:05 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Implements playback statistics on video and media elements (24.23 KB, patch)
2010-08-23 22:13 PDT, cajbir (:cajbir)
roc: review-
Details | Diff | Review
Patch: playback statistics v2 (50.94 KB, patch)
2010-12-07 13:23 PST, Chris Pearce (:cpearce)
no flags Details | Diff | Review
Patch v2 1/3: base stats (49.33 KB, patch)
2010-12-08 13:24 PST, Chris Pearce (:cpearce)
no flags Details | Diff | Review
Patch v2 2/3: Reduce paint delay in video playback (3.05 KB, patch)
2010-12-08 13:25 PST, Chris Pearce (:cpearce)
no flags Details | Diff | Review
Patch v2 3/3: Handle multiple frames per chunk per packet in WebM playback statistics. (1.13 KB, patch)
2010-12-08 13:29 PST, Chris Pearce (:cpearce)
kinetik: review+
Details | Diff | Review
Patch 1: Demuxing/decoding stats (27.70 KB, patch)
2011-02-23 16:53 PST, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Review
Patch 1 v2: Demuxing/decoding stats (18.30 KB, patch)
2011-02-27 13:13 PST, Chris Pearce (:cpearce)
kinetik: review+
roc: superreview+
Details | Diff | Review
Patch: Handle multiple frames per chunk per packet in WebM playback statistics. (1.13 KB, patch)
2011-02-28 17:47 PST, Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Review
Patch: add paint time to Images (15.11 KB, patch)
2011-03-01 16:47 PST, Chris Pearce (:cpearce)
no flags Details | Diff | Review
Patch: Implement mozFrameDelay and mozPaintCount (14.24 KB, patch)
2011-03-01 16:49 PST, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Review
Patch: add paint time to Images (15.11 KB, patch)
2011-03-01 17:56 PST, Chris Pearce (:cpearce)
no flags Details | Diff | Review
Patch: add paint stats to ImagesContainer (15.48 KB, patch)
2011-03-01 20:02 PST, Chris Pearce (:cpearce)
no flags Details | Diff | Review
Patch: Push ImageContainer subclass' locks up into a superclass monitor. (14.52 KB, patch)
2011-03-02 14:48 PST, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Review
Patch: add paint stats to ImagesContainer (10.42 KB, patch)
2011-03-02 14:51 PST, Chris Pearce (:cpearce)
no flags Details | Diff | Review

Description cajbir (:cajbir) 2010-07-20 23:14:04 PDT
It would be nice for JavaScript to be able to query a video element to see what framerate it is getting. This can be used for development, to see if improvements to video code improves the frame rate for example. 

It could be useful for general playback javascript to determine if a machine can playback a particular video size in a reasonable manner and switch to another size if it cannot.
Comment 1 cajbir (:cajbir) 2010-07-20 23:33:56 PDT
WHATWG thread about the topic:

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-May/026523.html

For testing bug 577843 I implemented a mozFrameCount so I could compute framerate of the playing video and compare playback performance with and without the scaling patch. I'll add the patch for that to this bug.

Another approach is mentioned in the above thread, instead of a frame count use dropped frames as the measure:

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-May/026528.html
Comment 2 cajbir (:cajbir) 2010-07-20 23:42:55 PDT
Created attachment 458935 [details]
HTML file using mozFrameCount to display framerate

Example HTML that uses mozFrameCount to display a framerate.
Comment 3 cajbir (:cajbir) 2010-07-20 23:44:36 PDT
Created attachment 458937 [details] [diff] [review]
Implement mozFrameCount on video elements
Comment 4 cajbir (:cajbir) 2010-08-12 22:28:50 PDT
Created attachment 465555 [details] [diff] [review]
Implements playback statistics on video and media elements

This updates the existing patch to trunk and adds:

- mozDroppedFrames to video element
- mozDownloadRate and mozPlaybackRate to media elements

The first gives us the same stat as Flash supplies. The second provides an alternative to the data that was removed from the progress event in bug 584615. These are also stats that have been asked for by people implementing live streaming bitrate fallback.
Comment 5 cajbir (:cajbir) 2010-08-12 22:29:44 PDT
Created attachment 465556 [details]
HTML file example to display dropped frames, download rate and other stats
Comment 6 Silvia Pfeiffer 2010-08-13 19:23:27 PDT
This is awesome! Can we have this in trunk, please? It would allow implementation of adaptive HTTP streaming in JavaScript!
Comment 7 Kevin Carle 2010-08-16 15:09:09 PDT
This is indeed awesome. Adaptive streaming stuff aside, this will be useful for us (YouTube) in just monitoring streaming performance / issues, especially as compared to Flash playback.
Comment 8 cajbir (:cajbir) 2010-08-23 20:23:06 PDT
Based on feedback I'm changing the names slightly:

HTMLMediaElement:
// The approximate rate at which the media resource is being downloaded in
// kilobytes per second. If the rate is not available (possibly due
// to not having downloaded enough data to compute a consistent value)
// this will be NaN.
readonly attribute float mozDownloadRate;

// The approximate rate at which the media resource is being decoded in
// kilobytes per second. If the rate is not available this will be
// NaN.
readonly attribute float mozDecodeRate;

HTMLVideoElement:
// A count of the number of frames that have been decoded and ready
// to be displayed. This can be used for computing the decoding framerate
readonly attribute unsigned long mozDecodedFrames;

// A count of the number of frames that have been dropped for performance
// reasons during playback.
readonly attribute unsigned long mozDroppedFrames;

So mozFrameCount becomes mozDecodedFrames and mozPlaybackRate becomes mozDecodeRate.
Comment 9 cajbir (:cajbir) 2010-08-23 22:05:03 PDT
Created attachment 468611 [details]
HTML file example to display dropped frames, download rate and other stats

Updated to use changed names.
Comment 10 cajbir (:cajbir) 2010-08-23 22:05:47 PDT
Created attachment 468612 [details] [diff] [review]
Implements playback statistics on video and media elements

Implements playback statistics.
Comment 11 cajbir (:cajbir) 2010-08-23 22:13:58 PDT
Created attachment 468617 [details] [diff] [review]
Implements playback statistics on video and media elements

Fixes small error in IDL comment. Rates are bytes per second, not kilobytes per second.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-23 22:51:57 PDT
Access to mDecodeDecodedFrames needs to be protected by a lock or something. Right now it's written on a decoder thread and read on the main thread.

I'm a bit concerned about the definitions of mozDecodedFrames and mozDroppedFrames. Currently mozDecodedFrames measures the number of frames presented by the decoder for display (not the same as the number of frames actually painted). mozDroppedFrames measures the number of frames dropped by the decoder, but doesn't count frames dropped while we sync up to the audio in AdvanceFrames, nor the frames that the decoder thread did SetCurrentImage on but which weren't actually painted.

I think maybe the way to go would be
* mozDecodedFrames: total number of frames seen by the decoder, including frames it skipped
* mozDisplayedFrames: total number of unique frames that we actually drew to the screen (as best we can tell)

The difference would be the number of frames we skipped for any reason.

But at this stage I think we should focus on FF4 blockers and get this in when we can fix it properly.
Comment 13 cajbir (:cajbir) 2010-08-24 20:09:41 PDT
(In reply to comment #12)
> Access to mDecodeDecodedFrames needs to be protected by a lock or something.
> Right now it's written on a decoder thread and read on the main thread.

It's protected by the mVideoUpdateLock lock when written but is missing it on the read. I'll fix that. Is that the right lock to use or do you want one specific for this purpose?

> mozDroppedFrames measures the number of frames dropped by
> the decoder, but doesn't count frames dropped while we sync up to the audio in
> AdvanceFrames, 

Yes, this is a bug. It should be adding to the dropped frames here.

> nor the frames that the decoder thread did SetCurrentImage on
> but which weren't actually painted.

This is because it is intended to measure whether the hardware can keep up with the decoding of the resource rather than the painting of it. This is useful for example when decoding the video and processing the video data via a canvas. I don't think this would be included in a count that only counted frames when painted.


> * mozDisplayedFrames: total number of unique frames that we actually drew to
> the screen (as best we can tell)

Possibly displayed frames is useful too but it doesn't address the canvas usecase I mention above i think.

I was considering just the decoding case as if painting can't keep up we'll no doubt see this reflected in the decoder skipping frames as well. Maybe both stats are needed.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-24 20:27:20 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > Access to mDecodeDecodedFrames needs to be protected by a lock or something.
> > Right now it's written on a decoder thread and read on the main thread.
> 
> It's protected by the mVideoUpdateLock lock when written but is missing it on
> the read. I'll fix that. Is that the right lock to use or do you want one
> specific for this purpose?

That's fine.

(In reply to comment #13)
> This is because it is intended to measure whether the hardware can keep up
> with the decoding of the resource rather than the painting of it. This is
> useful for example when decoding the video and processing the video data via
> a canvas. I don't think this would be included in a count that only counted
> frames when painted.

I think it would make sense to count canvas paints as "a paint" for the purpose of computing mozDisplayedFrames.

If we did that, are there any usecases for counting the number of frames that were presented as the ImageContainer's "current frame"?
Comment 15 cajbir (:cajbir) 2010-09-21 15:43:10 PDT
Robert, I need this  bug landed to meet our quarterly goals. Thanks.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-10-14 02:24:00 PDT
Comment on attachment 468617 [details] [diff] [review]
Implements playback statistics on video and media elements

We need to more precisely nail down what we measure here.
Comment 17 Chris Pearce (:cpearce) 2010-11-18 13:02:24 PST
I'll take this on. I'll need perf stats working before I start the refactorings in bug 592833 and friends.
Comment 18 Chris Pearce (:cpearce) 2010-11-21 19:33:00 PST
We discussed today what statistics we should report for our own internal perf testing. Here's what we came up with:

mozParsedFrames - number of frames that have been demuxed and extracted out of the media.
mozDecodedFrames - number of frames that have been decoded - converted into YCbCr.
mozPresentedFrames - number of frames that have been presented to the rendering pipeline for rendering - were "set as the current image".
mozPaintedFrames - number of frames which were presented to the rendering pipeline and ended up being painted on the screen. Note that if the video is not on screen (e.g. in another tab or scrolled off screen), this counter will not increase.
mozPaintDelay - the time delay between presenting the last frame and it being painted on screen (approximately).
Comment 19 Chris Pearce (:cpearce) 2010-12-07 13:23:52 PST
Created attachment 495942 [details] [diff] [review]
Patch: playback statistics v2

* Add the statistics I outlined in comment #18.
* Change the state machine thread's sleep time after presenting a frame, so that we sleep until it's time to display the next frame, rather than sleeping for the duration of the frame. Without this change, our presentation time gets later and later, as because of timer latency we gradually wake up later and later in the frame "period", and the "lateness" compounds over time.
* Minor change to nsWebMReader::DecodeVideoFrame()'s keyframe skipping logic to reflect that you can have multiple [key]frames per chunk per packet in a WebM file.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-07 14:51:27 PST
(In reply to comment #19)
> Created attachment 495942 [details] [diff] [review]
> Patch: playback statistics v2
> 
> * Add the statistics I outlined in comment #18.
> * Change the state machine thread's sleep time after presenting a frame, so
> that we sleep until it's time to display the next frame, rather than sleeping
> for the duration of the frame. Without this change, our presentation time gets
> later and later, as because of timer latency we gradually wake up later and
> later in the frame "period", and the "lateness" compounds over time.
> * Minor change to nsWebMReader::DecodeVideoFrame()'s keyframe skipping logic to
> reflect that you can have multiple [key]frames per chunk per packet in a WebM
> file.

Can you split these into separate patches? The latter two patches might be regressy so we should make sure we can land them (or back them out) separately.
Comment 21 Chris Pearce (:cpearce) 2010-12-08 13:24:49 PST
Created attachment 496253 [details] [diff] [review]
Patch v2 1/3: base stats

Adds playback statistics.
Comment 22 Chris Pearce (:cpearce) 2010-12-08 13:25:49 PST
Created attachment 496254 [details] [diff] [review]
Patch v2 2/3: Reduce paint delay in video playback
Comment 23 Chris Pearce (:cpearce) 2010-12-08 13:29:34 PST
Created attachment 496258 [details] [diff] [review]
Patch v2 3/3: Handle multiple frames per chunk per packet in WebM playback statistics.

The nsWebMReader::DecodeVideoFrame() function assumes that there's always exactly 1 frame per chunk per packet. This may not necessarily be the case, though usually it is, and it would be silly to mux a WebM file in any other way. This patch ensures that we can report the playback statistics correctly and keyframe skip correctly on such files.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-08 17:29:35 PST
You should probably split out the playback statistics patch into a gfx-only patch that provides the latest-time-stamp functionality and the rest.

+  // Number of presented frames which were drawn on screen.
+  readonly attribute unsigned long mozPaintedFrames;

You probably want to explain that presented frames may not be painted if the browser doesn't repaint the window frequently enough.

+   * Sets the time at which we'd like the contained to be image painted.

?

Why do we need to track anything in ImageContainer other than a single TimeStamp containing the last paint time for the current image (or null if it hasn't been painted yet)?

Why do an extra layer tree traversal in NotifyPainted? We can update the timestamp when we paint in each ImageLayer implementation.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-08 17:32:38 PST
Comment on attachment 496254 [details] [diff] [review]
Patch v2 2/3: Reduce paint delay in video playback

Wouldn't it be slightly simpler to have a local variable waitTime which is AUDIO_DURATION_MS by default and mPlayStartTime - mPlayDuration + TimeDuration::FromMilliseconds(videoData->mEndTime - mStartTime) - TimeStamp::Now() in the video case?
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-08 17:33:17 PST
Comment on attachment 496258 [details] [diff] [review]
Patch v2 3/3: Handle multiple frames per chunk per packet in WebM playback statistics.

Seems like more of a Matthew patch to me!
Comment 27 Matthew Gregan [:kinetik] 2011-02-14 19:37:01 PST
(In reply to comment #22)
> Created attachment 496254 [details] [diff] [review]
> Patch v2 2/3: Reduce paint delay in video playback

It'd be good to land this in FF4, it's a general bug fix.
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-14 20:14:25 PST
OK, someone needs to address comment #25 then
Comment 29 Chris Pearce (:cpearce) 2011-02-15 18:38:31 PST
(In reply to comment #27)
> (In reply to comment #22)
> > Created attachment 496254 [details] [diff] [review]
> > Patch v2 2/3: Reduce paint delay in video playback
> 
> It'd be good to land this in FF4, it's a general bug fix.

I'll spin this off into its own bug, and we can land it by itself.
Comment 30 Matthew Gregan [:kinetik] 2011-02-16 17:31:53 PST
(In reply to comment #29)
> I'll spin this off into its own bug, and we can land it by itself.

Bug 634787.
Comment 31 Chris Pearce (:cpearce) 2011-02-20 15:56:36 PST
So bug 633164 changed nsMediaDecoder::Invalidate() so that we only Invalidate() a video's frame if the image container size has changed. This causes us to call nsVideoFrame::BuildLayer() a lot less than once per painted frame (usually about 5 times per second for a 30fps video I'm testing on, though it varies). So I can't count the number of painted frames by counting the number of times that nsVideoFrame::BuildLayer() is called. How else can I count the number of video frames which get painted on screen?
Comment 32 Chris Pearce (:cpearce) 2011-02-23 14:35:11 PST
Comment on attachment 496254 [details] [diff] [review]
Patch v2 2/3: Reduce paint delay in video playback

The changes made in patch 2 were landed in bug 634787.
Comment 33 Chris Pearce (:cpearce) 2011-02-23 16:53:26 PST
Created attachment 514667 [details] [diff] [review]
Patch 1: Demuxing/decoding stats

I've split of the mozParsedFrames, mozDecodedFrames and mozPresentedFrames into this patch, since they're so much simpler to implement than mozPaintedFrames and mozFrameDelay. I'll implement those in subsequent patches.
Comment 34 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-23 17:53:46 PST
Comment on attachment 514667 [details] [diff] [review]
Patch 1: Demuxing/decoding stats

Looks good to me but I want Matthew to check too.
Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-23 17:54:13 PST
Comment on attachment 514667 [details] [diff] [review]
Patch 1: Demuxing/decoding stats

Yes, I know Matthew technically isn't a super-reviewer :-)
Comment 36 Matthew Gregan [:kinetik] 2011-02-23 18:13:01 PST
+/* readonly attribute unsigned long mozDecodedFrames; */
+NS_IMETHODIMP nsHTMLVideoElement::GetMozParsedFrames(PRUint32 *aMozDecodedFrames)

Comment and parameter name don't match member function name.  Same for the dropped/decoded.  And presented is missing a similar comment.

Also, if mDecoder is null, doesn't it make more sense to return an error from these getters?

+        PRUint32 parsed=0, decoded=0;

Pleaseusespaces.  (Same issue in a second place, too.)

+  virtual PRBool DecodeVideoFrame(PRBool& aKeyframeSkip,
+                                  PRInt64 aTimeThreshold,
+                                  PRUint32& aParsed,
+                                  PRUint32& aDecoded) = 0;

I think I'd prefer aParsed/aDecoded to be wrapped in a new stats struct and passed in as a single item, especially if we're going to add more in the future.

As I read further, I see the stats have their own monitor, so this should definitely all be in a new struct with its own monitor, and passed around/fetched similar to the existing network/playback statistics.

+#define IMPL_GET_STAT_METHOD(X) \
+PRUint32 nsMediaDecoder::Get##X##Frames() { \
+  mozilla::MonitorAutoEnter mon(mStatsMonitor); \
+  return m##X##Frames; \
+}
+
+IMPL_GET_STAT_METHOD(Parsed);
+IMPL_GET_STAT_METHOD(Decoded);
+IMPL_GET_STAT_METHOD(Presented);

This seems unnecessary for three tiny methods, and it makes grepping for stuff a lot harder.  Moving to a GetPlaybackStatistics() method like I suggested above would remove the need for separate getters, anyway.
Comment 37 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-23 18:20:49 PST
(In reply to comment #36)
> Also, if mDecoder is null, doesn't it make more sense to return an error from
> these getters?

I think probably not. If there is no decoder, then 0 is the correct answer.

> As I read further, I see the stats have their own monitor, so this should
> definitely all be in a new struct with its own monitor, and passed
> around/fetched similar to the existing network/playback statistics.

Actually, do we need the stats to have their own monitor? Why can't they use the decoder monitor?
Comment 38 Matthew Gregan [:kinetik] 2011-02-23 18:28:53 PST
(In reply to comment #37)
> I think probably not. If there is no decoder, then 0 is the correct answer.

Is that true if we've been playing and then DecodeError fires?
Comment 39 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-23 18:43:48 PST
We can say that the counters are reset if a DecodeError fires.
Comment 40 Chris Pearce (:cpearce) 2011-02-23 19:06:44 PST
Thanks for your comments Matthew and Roc.

(In reply to comment #37)
> (In reply to comment #36)
> > Also, if mDecoder is null, doesn't it make more sense to return an error from
> > these getters?
> 
> I think probably not. If there is no decoder, then 0 is the correct answer.

Yeah, if we don't have a decoder, we'll have decoded 0 frames.

> Actually, do we need the stats to have their own monitor? Why can't they use
> the decoder monitor?

IIRC (and it was at least month since I tested this) when mozPaintedFrames and mozFrameDelay was implemented as per my original patch, using the decoder monitor caused a slight frame rate drop.

(In reply to comment #39)
> We can say that the counters are reset if a DecodeError fires.

We only fire DecodeError (in the nsBuiltinDecoder architecture) upon failure in LoadMetadata(), so it's unlikely the counters will be very useful in this situation anyway.

In the case where we've been playing and a DecodeError fires, it would make sense to keep the stats around, since we will have decoded and presented frames, but that can't happen currently.
Comment 41 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-23 19:32:33 PST
(In reply to comment #40)
> > Actually, do we need the stats to have their own monitor? Why can't they use
> > the decoder monitor?
> 
> IIRC (and it was at least month since I tested this) when mozPaintedFrames and
> mozFrameDelay was implemented as per my original patch, using the decoder
> monitor caused a slight frame rate drop.

OK. Then I agree with Matthew, we should have a struct with a monitor containing the values it protects.
Comment 42 Chris Pearce (:cpearce) 2011-02-27 13:13:23 PST
Created attachment 515506 [details] [diff] [review]
Patch 1 v2: Demuxing/decoding stats

Adjusted with Matthews comments. Re-requesting review, since it's changed significantly.
Comment 43 Chris Pearce (:cpearce) 2011-02-28 17:47:51 PST
Created attachment 515799 [details] [diff] [review]
Patch: Handle multiple frames per chunk per packet in WebM playback statistics.

Unbitrotten, based upon latest trunk and new Patch 1. Carrying forward r=kinetik.
Comment 44 Chris Pearce (:cpearce) 2011-03-01 13:15:37 PST
(In reply to comment #24)
> Why do we need to track anything in ImageContainer other than a single
> TimeStamp containing the last paint time for the current image (or null if it
> hasn't been painted yet)?

If we only track the paint time of the current image (or null if it's not been painted yet) on the ImageContainer, what do we return in HTMLMediaElement.mozFrameDelay when the current image has not been painted yet? 0? But then we couldn't distinguish between no delay (e.g. perfect perf) and when we'd sampled before the frame had been painted. Alternatively if we returned the last returned value in this case, we may never get a non-zero value if we always poll before the current frame has been painted.

If we store the paint target time on the ImageContainer, then we know what the delay is, and we can retain the delay of the previous image, and return that when the current image has not been painted yet.

Alternatively we could have some kind of notification on the nsMediaDecoder when an Image is painted, and maintain the delay of the last painted frame there?
Comment 45 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-01 13:47:26 PST
(In reply to comment #44)
> If we only track the paint time of the current image (or null if it's not been
> painted yet) on the ImageContainer, what do we return in
> HTMLMediaElement.mozFrameDelay when the current image has not been painted yet?
> 0?

It should return the most recent delay. However, we can achieve this by having the video decoder store the last frame's delay internally just before it does SetCurrentImage on the next frame. Then mozFrameDelay can call into the decoder to get the delay, and that will return the current frame's delay if that's non-null, otherwise the last frame's delay.
Comment 46 Chris Pearce (:cpearce) 2011-03-01 16:47:38 PST
Created attachment 516082 [details] [diff] [review]
Patch: add paint time to Images

* Adds paint time attribute to Image.
Comment 47 Chris Pearce (:cpearce) 2011-03-01 16:49:22 PST
Created attachment 516083 [details] [diff] [review]
Patch: Implement mozFrameDelay and mozPaintCount

Implement mozFrameDelay and mozPaintCount. Not quite sure it's ready for review, posting so cjones can see what I'm doing to give me feedback on how to do this for shadow layers.
Comment 48 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-01 17:19:43 PST
Currently the PLayers protocol is all one-way, child->parent (content->compositor).  I think the cleanest way to implement statistics like this would be adding feedback parent->child on painting.  This is potentially useful for other stuff too.

So to http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/PLayers.ipdl#239, I'd suggest adding a

child:
  Painted(PaintStats[] stats);

message.  PaintStats could be defined as a union of more specific stats, like http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/PLayers.ipdl#139.  You would want to attach a PLayer to the specific stats, to know which Layer to notify in the child.

To collect the stats, you would probably want to create a compositor-side interface in http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.cpp for ShadowLayerManager, modeled after the Transaction stuff for ShadowLayerForwarder.  The compositor-side layer managers would "open" stats gathering at the beginning of a transaction, then "close" it at the end.  (You would still want to close them for empty layers transactions, unlike the ShadowLayerForwarder Transactions which remain open.)  Closing stats gathering would cause an IPC message to be sent to the child.  If no stats were collected, no message would need to be sent.

On the child side, you'd want to iterate though the stats and update individual layers.  If all these stats are layer-backend agnostic, you could add generic processing like in http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayersParent.cpp#161.  If they might be backend-specific, we'd need something like http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicLayers.cpp#2793.

Some asides
 - It will be possible for PLayer:__delete__ to race with these Painted() messages, which will make crashy.  We'll need to fix this by adding another step to the PLayer deletion protocol: add a PLayer:Destroy child->parent message that ends up triggering PLayer:__delete__ parent->child.
 - I think we'll need that for PLayers too.

I'm not sure how coherent all that is.  Let me know if I can help more.  Also, IMHO landing same-process stats shouldn't block on cross-process stats.
Comment 49 Chris Pearce (:cpearce) 2011-03-01 17:56:44 PST
Created attachment 516105 [details] [diff] [review]
Patch: add paint time to Images

Fixed two typo in comments, ready for review.
Comment 50 Chris Pearce (:cpearce) 2011-03-01 17:57:52 PST
Comment on attachment 516083 [details] [diff] [review]
Patch: Implement mozFrameDelay and mozPaintCount

Note this patch is based on top of the patch "add paint time to Images".
Comment 51 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-01 18:14:31 PST
I don't think we need the mPainted flag on the Image. We only need a flag on the ImageContainer to say whether the current image has been painted yet. In fact we don't even need a separate flag, since mPaintTime is null if and only if the current image hasn't been painted yet.
Comment 52 Chris Pearce (:cpearce) 2011-03-01 18:54:22 PST
Note mPaintCount counts the number of Images which have been painted at least once, and video frames often end up being painted several times.

We need the mPainted flag on the Image because it may lose it's "current image" status while painting, and in that case we still need to increment mPaintCount, but not set mPaintTime if this was the first time that Image was painted. But we can't tell if this is the first time we've painted the (now non-current) Image if we reset mPaintTime when the current image changed.

If an Image has been painted, and the current image changes while we were painting the Image for a second time, mPaintTime will be reset. If we assume we should increment mPaintCount because mPaintTime is null, we'll end up incrementing mPaintCount twice for that frame.
Comment 53 Chris Pearce (:cpearce) 2011-03-01 19:06:39 PST
We could instead have a flag on the ImageContainer to store whether its previous image was painted. That basically would need to be set to !mPaintTime.IsNull() when the current image changes, that would handle the problematic case.
Comment 54 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-01 19:48:41 PST
Let's do that then.
Comment 55 Chris Pearce (:cpearce) 2011-03-01 20:02:55 PST
Created attachment 516139 [details] [diff] [review]
Patch: add paint stats to ImagesContainer
Comment 56 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-01 21:14:54 PST
+   * Implementations must call CurrentImageChanged() in a threadsafe manner.

What does "in a threadsafe manner" mean?

+   * has not yet been painted.  Threadsafe.

I think it's clearer to say "can be called on any thread".

+  PRBool mPreviousImagePainted;

PRPackedBool

Maybe we should just bite the bullet and move the monitor in each implementation up to the superclass?
Comment 57 Chris Pearce (:cpearce) 2011-03-02 14:48:54 PST
Created attachment 516418 [details] [diff] [review]
Patch: Push ImageContainer subclass' locks up into a superclass monitor.

Merging ImageContainers' monitors are suggested.
Comment 58 Chris Pearce (:cpearce) 2011-03-02 14:51:13 PST
Created attachment 516419 [details] [diff] [review]
Patch: add paint stats to ImagesContainer

Review comments addressed. Based on top of "Patch: Push ImageContainer subclass' locks up into a superclass monitor.".
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-03 12:28:15 PST
Comment on attachment 516418 [details] [diff] [review]
Patch: Push ImageContainer subclass' locks up into a superclass monitor.

Looks great, but please change the comments in ImageLayers.h to mention the monitor and say which methods take the monitor.
Comment 61 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-04-01 11:52:41 PDT
The following is what I see in the latest nightly:
Framerate is: 30.01
Dropped frames: undefined
Download rate: NaN
Playback rate: NaN

Is this expected?
Comment 62 David Humphrey (:humph) 2011-04-01 11:59:49 PDT
        dropped.innerHTML = v1.mozDroppedFrames;
        playback.innerHTML = Math.round(v1.mozDecodeRate);
        download.innerHTML = Math.round(v1.mozDownloadRate);

Probably just typos, since these properties don't exist on the video element.  However, I can verify for you that these stats are indeed working on the video element, as I'm working on a visualization that uses them now.
Comment 63 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-04-01 12:01:41 PDT
Thanks, David.
Comment 64 Chris Pearce (:cpearce) 2011-04-01 12:12:09 PDT
Comment on attachment 468611 [details]
HTML file example to display dropped frames, download rate and other stats

That example is for the original stats produced by the original patch. For a demo which uses the current stats, see:
http://people.mozilla.org/~cpearce/paint-stats-demo.html
Comment 65 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-04-01 12:34:02 PDT
(In reply to comment #64)
> Comment on attachment 468611 [details]
> HTML file example to display dropped frames, download rate and other stats
> 
> That example is for the original stats produced by the original patch. For a
> demo which uses the current stats, see:
> http://people.mozilla.org/~cpearce/paint-stats-demo.html

Looks good to me.

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