Bug 170024 - Fix framesEncoded/framesDecoded RTC stats
Summary: Fix framesEncoded/framesDecoded RTC stats
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 169662
  Show dependency treegraph
 
Reported: 2017-03-23 14:16 PDT by youenn fablet
Modified: 2017-03-24 09:25 PDT (History)
3 users (show)

See Also:


Attachments
Patch (7.24 KB, patch)
2017-03-23 14:18 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (7.24 KB, patch)
2017-03-24 08:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-03-23 14:16:47 PDT
Fix framesEncoded/framesDecoded RTC stats
Comment 1 youenn fablet 2017-03-23 14:18:18 PDT
Created attachment 305226 [details]
Patch
Comment 2 Eric Carlson 2017-03-23 17:26:25 PDT
Comment on attachment 305226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305226&action=review

> LayoutTests/webrtc/video-stats.html:86
> +        return waitFor(1000);

1) hard coded timeout seems like a recipe for a flakey test, and 2) this means the test will always take a full second to run. Is there any way to have it check for completion instead of always waiting a second?

> LayoutTests/webrtc/video-stats.html:95
> +        assert_true(stats.timestamp > statsFirstConnection.timestamp, "Timestamp should have increased");
> +        assert_true(stats.framesEncoded > statsFirstConnection.framesEncoded, "Number of encoded frames should have increased");
> +        return getInboundRTPStats(secondConnection);
> +    }).then((stats) => {
> +        assert_true(stats.timestamp > statsSecondConnection.timestamp, "Timestamp should have increased");
> +        assert_true(stats.framesDecoded > statsSecondConnection.framesDecoded, "Number of decoded frames should have increased");

Having the same failure message for two different tests will make this harder to debug if it fails. Can you make the messages different, eg. "Timestamp should have increased in first connection stats"?
Comment 3 youenn fablet 2017-03-23 18:16:04 PDT
Thanks for the review.

> > LayoutTests/webrtc/video-stats.html:86
> > +        return waitFor(1000);
> 
> 1) hard coded timeout seems like a recipe for a flakey test, and 2) this
> means the test will always take a full second to run. Is there any way to
> have it check for completion instead of always waiting a second?

We need to let some time for the "cached stats report" to be obsolete and a new one to be created.
But we also need to ensure there are some new frames decoded and encoded to ensure we are not flaky.
1000 might be a bit too large, 100 is a minimum.
I will reduce it to 200.

> 
> > LayoutTests/webrtc/video-stats.html:95
> > +        assert_true(stats.timestamp > statsFirstConnection.timestamp, "Timestamp should have increased");
> > +        assert_true(stats.framesEncoded > statsFirstConnection.framesEncoded, "Number of encoded frames should have increased");
> > +        return getInboundRTPStats(secondConnection);
> > +    }).then((stats) => {
> > +        assert_true(stats.timestamp > statsSecondConnection.timestamp, "Timestamp should have increased");
> > +        assert_true(stats.framesDecoded > statsSecondConnection.framesDecoded, "Number of decoded frames should have increased");
> 
> Having the same failure message for two different tests will make this
> harder to debug if it fails. Can you make the messages different, eg.
> "Timestamp should have increased in first connection stats"?

Right, will do
Comment 4 youenn fablet 2017-03-24 08:43:10 PDT
Created attachment 305285 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2017-03-24 09:25:12 PDT
Comment on attachment 305285 [details]
Patch for landing

Clearing flags on attachment: 305285

Committed r214348: <http://trac.webkit.org/changeset/214348>
Comment 6 WebKit Commit Bot 2017-03-24 09:25:15 PDT
All reviewed patches have been landed.  Closing bug.