Open Bug 686370 Opened 13 years ago Updated 2 years ago

Implement video playback statistics proposed in WHATWG

Categories

(Core :: Audio/Video: Playback, defect, P2)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

People

(Reporter: cpearce, Unassigned)

References

(
URL
)

Details

Attachments

(1 file)

Initial PlaybackJitter Patch
9.04 KB, patch
cpearce
: review-
Details | Diff | Splinter Review 
There's a pretty stable and accepted specification at http://wiki.whatwg.org/wiki/Video_Metrics for stats to be presented for video/audio elements. They're similar to our existing stats.

See also http://openetherpad.org/ovc11-standards-for-browser-video-statistics for discussion, and http://www.w3.org/Bugs/Public/show_bug.cgi?id=12399 .
I've got some students that might like to work on this in January if it's not done by then ;)
Hey Humph,

After our discussion in class I'd like to work on this bug.  Mind assigning it to me?
David Seifried, my original implementation of this was done in bug 580531, my patches/changesets in that bug should give you an idea how to implement this.
Thanks Chris, I'll be sure to take a look.
Assignee: nobody → david.c.seifried
Status: NEW → ASSIGNED
Couple of things:

1. I'm fairly certain the data types I'm using the the attribute itself are screwed up. As far as I could tell in the spec it wasn't too specific on this front.
2. As you will notice in the patch, I left a little "thisWillNotPass" hint for the Ogg stuff as I wasn't sure how to approach this from that end since it makes calls to this same function.


Anyway, any suggestions/criticisms are welcome! Let me know how badly I screwed up :p
Attachment #616157 - Flags: review?(cpearce)
Comment on attachment 616157 [details] [diff] [review]
Initial PlaybackJitter Patch

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

You've made a good start, but there's still plenty to do, keep it up! :)

Are you going to attempt to implement the other metrics (like bytesReceived etc)? You'd be best to tackle one at a time though.

::: content/media/VideoFrameContainer.cpp
@@ +17,5 @@
>  
>  void VideoFrameContainer::SetCurrentFrame(const gfxIntSize& aIntrinsicSize,
>                                            Image* aImage,
> +                                          TimeStamp aTargetTime,
> +                                          PRInt64 aIntendedDuration)

aIntendedDuration should be a TimeDuration.

@@ +38,5 @@
>      mImageSizeChanged = true;
>    }
>  
> +  if (!lastPaintTime.IsNull() && !aTargetTime.IsNull()) {
> +    mPlaybackJitter += (aIntendedDuration - (aTargetTime - lastPaintTime).ToMilliseconds());

So playback jitter is defined at http://wiki.whatwg.org/wiki/Video_Metrics#playbackJitter as:

playbackJitter = sum(abs(Ei - Ai)) 

where:
Ei = Desired duration of frame i spent on the screen (to nearest microsecond?) 
Ai = Actual duration frame i spent on the screen (if the frame is never presented to the user, then Ai == 0). 

(On most platforms we care about the timer resolution are limited to millisecond precision, so don't worry about microsecond position).

Also aTargetTime is the TimeStamp at which we should paint aImage, whereas lastPaintTime is the time at which the ImageContainer's *previous* image was painted. So the duration which the *previous* image was on screen for is (TimeStamp::Now() - lastPaintTime), and the previous image's duration was passed in last time this function was called.

So I think you should:
1. Add a new field to VideoFrameContainer, TimeDuration mLastFrameIntendedDuration.
2. When you enter SetCurrentFrame(), if we have a non-null mLastFrameIntendedDuration, then the jitter for the previous frame was:
TimeDuration frameJitter = (TimeStamp::Now() - lastPaintTime) - mLastFrameIntendedDuration;
Calculate this and add it to mPlaybackJitter (which can stay as a double, we return it to JS as a double).
3. Set mLastFrameIntendedDuration to aIntendedDuration.

::: content/media/VideoFrameContainer.h
@@ +43,5 @@
>      NS_ASSERTION(mImageContainer, "aContainer must not be null");
>    }
>    // Call on any thread
>    void SetCurrentFrame(const gfxIntSize& aIntrinsicSize, Image* aImage,
> +                       TimeStamp aTargetTime, PRInt64 aIntendedDuration);

aIntendedDuration should be a TimeDuration, not an PRInt64.

@@ +48,5 @@
>    // Time in seconds by which the last painted video frame was late by.
>    // E.g. if the last painted frame should have been painted at time t,
>    // but was actually painted at t+n, this returns n in seconds. Threadsafe.
>    double GetFrameDelay();
> +  // 

Need a meaningful comment.

@@ +76,5 @@
>    // The delay between the last video frame being presented and it being
>    // painted. This is time elapsed after mPaintTarget until the most recently
>    // painted frame appeared on screen.
>    TimeDuration mPaintDelay;
> +  // TODO: Come up with a good comment.

Yes, you totally should. ;)

A link to the wiki/spec would be good to include.

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +1879,5 @@
>      return;
>    }
>  
> +  // Assuming that the times retrieved from the VideoData is the intended times for the frame
> +  PRInt64 intendedDuration = aData->mEndTime - aData->mTime;

I'd rather you used TimeDuration.

::: content/media/ogg/nsOggReader.cpp
@@ +272,4 @@
>          container->SetCurrentFrame(gfxIntSize(displaySize.width, displaySize.height),
>                                     nsnull,
> +                                   TimeStamp::Now(),
> +                                   thisWillNotPass);

Hmm, we'll need a way to not include video frame durations from frames that are displayed while we're not playing (paused, seeking etc). Otherwise they'll skew the jitter.

How about when we call SetCurrentFrame we pass in a boolean parameter to designate whether we paused since the last paint. We know if we were playing for the entire duration the previous frame was displayed; if we weren't, we shouldn't include its jitter in the jitter sum. You'll then need to figure out how to track this in the nsBuiltinDecoderStateMachine.
Attachment #616157 - Flags: review?(cpearce) → review-
Comment on attachment 616157 [details] [diff] [review]
Initial PlaybackJitter Patch

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

::: content/media/ogg/nsOggReader.cpp
@@ +272,4 @@
>          container->SetCurrentFrame(gfxIntSize(displaySize.width, displaySize.height),
>                                     nsnull,
> +                                   TimeStamp::Now(),
> +                                   thisWillNotPass);

Argh, was a but trigger happy on the review for this. What I meant to say is that you need to figure out if we were paused at some point during the last frame, and I think tracking whether we paused (whether nsBuiltinDecoderStateMachine::StopPlayback() was called) since the last call to SetCurrentFrame() is an easy way to achieve this.
> Are you going to attempt to implement the other metrics (like bytesReceived
> etc)? You'd be best to tackle one at a time though.

Yeap we plan on finishing them all :) I have bytesDecoded, bytesReceived, and droppedFrames ( this one only sort of ) working. Do you want me to put those up for review or wait until I get the others done as well?
When you think they're ready for review put them up. That way you can parallelize; your work is less likely to be totally blocked waiting on me to review them all at once.
Alright will do
Would be nice to add these to the default videocontrol stats (as implemented in bug 669260). Followup?
Pokey pokey. Progress?
Component: Audio/Video → Audio/Video: Playback
Rank: 15
Priority: -- → P2

The bug assignee didn't login in Bugzilla in the last 7 months.
:jimm, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: david.c.seifried → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)

<a href="https://neekincest.com/category/%d8%a7%d9%81%d9%84%d8%a7%d9%85-%d9%86%d9%8a%d9%83/" > افلام نيك</a> - <a href="https://neekincest.com/category/xnxx/" > xnxx</a> - <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d8%a7%d8%ac%d9%86%d8%a8%d9%89/" > سكس اجنبي</a> - <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d8%a7%d8%ae%d9%88%d8%a7%d8%aa/" > اخوات</a> - <a href="https://neekincest.com/category/%d9%82%d8%b5%d8%b5-%d8%b3%d9%83%d8%b3/" > قصص سكس</a>

<a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d8%a7%d9%85%d8%b1%d9%8a%d9%83%d9%8a/" > سكس امريكي</a> - <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d8%a7%d9%85%d9%87%d8%a7%d8%aa/" > سكس امهات</a> - <a href="https://neekincest.com/category/%d8%b5%d9%88%d8%b1-%d8%b3%d9%83%d8%b3/" > صور سكس</a> - <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d8%ac%d8%af%d9%8a%d8%af/" > سكس جديد</a> - <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d8%ad%d9%8a%d9%88%d8%a7%d9%86%d8%a7%d8%aa/" > سكس حيوانات
</a>

<a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d8%b3%d8%b9%d9%88%d8%af%d9%89/" > سكس سعودى</a> - <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d8%b3%d8%ad%d8%a7%d9%82/" > سكس سحاق</a> - <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d8%b2%d9%86%d8%ac%d9%89/" > سكس زنجى</a> - <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d9%83%d9%84%d8%a7%d8%a8/" > سكس كلاب</a> - <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d8%ae%d9%84%d9%81%d9%89/" > سكس خلفى</a>

<a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d9%85%d8%b5%d8%b1%d9%89/" > سكس مصرى</a> - <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d9%85%d8%ad%d8%a7%d8%b1%d9%85/" > سكس محارم</a> - <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d8%b9%d8%b1%d8%a8%d9%89/" > سكس عربى</a>  _ <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d9%85%d8%ad%d8%ac%d8%a8%d8%a7%d8%aa/" > سكس محجبات</a>

<a href="https://neekincest.com/category/%d9%86%d9%8a%d9%83-%d9%85%d8%b1%d8%a7%d9%87%d9%82%d8%a7%d8%aa/" > نيك مراهقات
</a> _ <a href="https://neekincest.com/category/%d9%86%d9%8a%d9%83-%d8%b7%d9%8a%d8%b2/" > نيك طيز</a> _ <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d9%83%d8%b3/" > سكس كس</a> _ <a href="https://neekincest.com/category/%d8%b3%d9%83%d8%b3-%d9%85%d8%aa%d8%b1%d8%ac%d9%85/" > سكس مترجم</a>

<a href="https://xnxxarby.com/category/%d8%a7%d9%81%d9%84%d8%a7%d9%85-%d8%b3%d9%83%d8%b3/" >افلام سكس</a> - <a href="https://xnxxarby.com/category/xnxx/" >xnxx</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d8%a7%d8%ac%d9%86%d8%a8%d9%89/" >سكس اجنبى</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d8%a7%d8%ae%d9%88%d8%a7%d8%aa/" >سكس اخوات</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d8%ad%d9%8a%d9%88%d8%a7%d9%86%d8%a7%d8%aa/" >سكس حيوانات</a>

<a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d8%b7%d9%8a%d8%b2/" >سكس طيز</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d8%b3%d8%ad%d8%a7%d9%82/" >سكس سحاق</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d8%b2%d9%86%d9%88%d8%ac/" >سكس زنوج</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d8%a7%d9%85%d9%87%d8%a7%d8%aa/" >سكس امهات</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d8%a7%d9%85%d8%b1%d9%8a%d9%83%d9%89/" >سكس امريكي</a>

<a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d9%85%d8%ad%d8%a7%d8%b1%d9%85/" >سكس محارم</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d9%85%d8%ac%d8%a7%d9%86%d9%89/" >سكس مجانى</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d9%81%d8%b1%d9%86%d8%b3%d9%89/" >سكس فرنسي</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d8%b9%d8%b1%d8%a8%d9%89/" >سكس عربى</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d8%b9%d8%a7%d9%84%d9%85%d9%89/" >سكس عالمى</a>

<a href="https://xnxxarby.com/category/%d9%82%d8%b5%d8%b5-%d8%b3%d9%83%d8%b3/" >قصص سكس</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d9%85%d8%b5%d8%b1%d9%89/" >سكس مصرى</a> - <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d9%85%d8%ad%d8%ac%d8%a8%d8%a7%d8%aa/" >سكس محجبات</a> _ <a href="https://xnxxarby.com/category/%d8%b5%d9%88%d8%b1-%d8%b3%d9%83%d8%b3/" >صور سكس</a> _ <a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d9%83%d9%84%d8%a7%d8%a8/" >سكس كلاب</a>
<a href="https://xnxxarby.com/category/%d8%b3%d9%83%d8%b3-%d9%85%d8%aa%d8%b1%d8%ac%d9%85/" >سكس مترجم</a>

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: