Bug 170024 – Fix framesEncoded/framesDecoded RTC stats

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Fix framesEncoded/framesDecoded RTC stats

[youenn fablet]

Created attachment 305226 [details]
Patch

[Eric Carlson]

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"?

[youenn fablet]

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

[youenn fablet]

Created attachment 305285 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 305285 [details]
Patch for landing

Clearing flags on attachment: 305285

Committed r214348: <http://trac.webkit.org/changeset/214348>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.