WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
170024
Fix framesEncoded/framesDecoded RTC stats
https://bugs.webkit.org/show_bug.cgi?id=170024
Summary
Fix framesEncoded/framesDecoded RTC stats
youenn fablet
Reported
2017-03-23 14:16:47 PDT
Fix framesEncoded/framesDecoded RTC stats
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2017-03-23 14:18:18 PDT
Created
attachment 305226
[details]
Patch
Eric Carlson
Comment 2
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"?
youenn fablet
Comment 3
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
youenn fablet
Comment 4
2017-03-24 08:43:10 PDT
Created
attachment 305285
[details]
Patch for landing
WebKit Commit Bot
Comment 5
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
>
WebKit Commit Bot
Comment 6
2017-03-24 09:25:15 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug