New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 681832 link

Starred by 4 users

Issue metadata

Status: WontFix
Owner: ----
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

41.3%-50% regression in browser_tests at 442070:442081

Project Member Reported by nisse@chromium.org, Jan 17 2017

Issue description

See the link to graphs below.
 

Comment 2 by nisse@chromium.org, Jan 17 2017

Owner: kjellander@chromium.org
Status: Assigned (was: Untriaged)
Large change in unique frames and max repeated on mac. The change looks quite similar to https://bugs.chromium.org/p/chromium/issues/detail?id=681815, but it is associated with a commit position some 150 units earlier, apparently with no video or webrtc related changes.

Comment 3 by nisse@chromium.org, Jan 17 2017

Cc: chfremer@chromium.org
Cc: emir...@chromium.org
Cc: kjellander@chromium.org
Owner: mcasas@chromium.org
I ran the test manually on my Linux dev box and found that the drop starts with commit https://chromium.googlesource.com/chromium/src/+/50252de90c71b7d5d12c30121a82d4e790f74252, which is "WebmMuxer: do not set DefaultDuration if timestamp delta is not near constant".

On this revision the test outputs
RESULT Unique_frames_count: 720p_VP8= 83 score
On the revision before it outputs
RESULT Unique_frames_count: 720p_VP8= 141 score

mcasas@: Could you have a look at this, please? Also issue 681815, which is most likely related.


Comment 6 by mcasas@chromium.org, Jan 17 2017

Issue 681815 has been merged into this issue.

Comment 7 by mcasas@chromium.org, Jan 17 2017

Owner: ehmaldonado@chromium.org
IIRC the video/audio quality browser tests use MediaRecorder as
a way to record the output of a PeerConnection for quality post
comparison.  The mentioned CL was needed to make MSE work nicely
with MR, and should not have affected external players such as 
e.g. VLC.  So I'm tempted to mark this as a WontFix and let the
bots gain their new ground.  

However, I'd like to ask phoglund@ and/or ehmaldonado@ since they 
wrote the Webm postprocessing functions in [1,2].


[1] https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_video_quality_browsertest.cc?q=WriteCapturedWebmVideo&sq=package:chromium&l=131&dr=CSs
[2] https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_video_quality_browsertest.cc?q=RunWebmToI420Converter&sq=package:chromium&l=145&dr=CSs

Those functions write the Webm video to a file, and then convert it to I420 using ffmpeg. I'm not sure if that would affect the output that ffmpeg produces.
Cc: ehmaldonado@chromium.org
Owner: mcasas@chromium.org
Since we have a major regression on unique frames (much fewer) it tells me performance has gone a lot worse - we simply cannot keep up the pace, which is serious.

The fact that max_repeated frames drops is normally a good thing, but likely cause simply by the fact that fewer frames are able to make it through the test.

Can we just revert https://chromium.googlesource.com/chromium/src/+/50252de90c71b7d5d12c30121a82d4e790f74252 and see if the graphs recover?
I would strongly advise against reverting: the CL was legitimate and
solved a real problem when playing back the video, in Chrome. VLC is
still playing fine the same video, so in this case I'd say our test
infra is not working correctly.
Components: Blink>WebRTC>Tools
Owner: ehmaldonado@chromium.org
Assuming Miguel has no plans to isolate the culprit, I'm reassigning to Edward. 
Can you run the test (Linux is fine, since it regresses on all platforms) and see if you can see what's causing the performance slowdown?

Locally reverting the mentioned CL could also be interesting, to see if it solves everything.
Cc: holmer@chromium.org peah@chromium.org
 Issue 687051  has been merged into this issue.
Cc: mcasas@chromium.org
 Issue 679366  has been merged into this issue.

Comment 15 by tommi@chromium.org, Mar 14 2017

Edward - is there something left to be done for this issue?
I still haven't got around to do what Henrik suggested in #12
I reverted that CL locally, and it didn't have any impact.
No, sorry, I was mistaken. Reverting it locally does solve everything.

Miguel: Sorry for bringing this up again so late. Do you think this regression is a WebRTC regression?
#19: I think this is an issue with the test set up, most 
likely with the ffmpeg scripting used to extract the 
individual frames out of the recording.  We should start
digging there.  Try to get the recorded video (start by
finding the ondataavailable JS event listener).
Re #20: What makes you think that? These tests have been running stable for years and the tooling part dealing with ffmpeg has never been the cause of any flakiness we've seen. The behavior of the test clearly changed in https://chromium.googlesource.com/chromium/src/+/50252de90c71b7d5d12c30121a82d4e790f74252 but why would that have to do with ffmpeg? 
Does that tiny code change alter the timing of which frames are recorded into the .webm file or something like that? Then I'd like to understand how you can claim that is not a regression that should be reverted, especially as a revert makes our test go back to the much better performance numbers.
Re #21:
Does that tiny code change alter the timing of which frames are recorded into the .webm file or something like that?

It does look like it (from  crbug.com/606000 ):

> #22:
> The issue is a discontinuity as Matt said. The video frame at time 1.601 is
> marked as having duration .033. The next video frame is at time 1.68. 
>
> 1.68 - 1.601 = 0.079
>
> So either the time of 1.68 is wrong or the duration of .033 is wrong. The
> time is read from the timecode field of this block. The block does not set an
> explicit duration, so we look for a default duration. In this case the video
> track has a default duration of .033.
>
> If the track did not have a default duration, we would have set the duration
> to be the difference in timestamps between current and next buffer. I've
> verified that if the duration is set this way (commenting out reading of
> default duration in chrome), the video plays back fine. 

> #31:
> I think this should be a pretty easy fix - simply stop setting track DefaultDuration.

Hence the change. So this makes me believe this is an issue that only affects the test toolchain for some reason, and doesn't imply a regression in WebRTC.

Miguel: Would you like to have access to some recorded .webm video?
Components: -Blink>WebRTC>Tools Blink>WebRTC>Video
Correcting the component. (Blink>WebRTC>Tools is not for infra/test issues.)
ehmaldonado@ are you currently looking at this?
I'm tempted to close this as Won't Fix.
kjellander: What do you think?
I don't want to close this just blaming "something is wrong with the toolchain". I'd like to see proof on how that can be and an explanation on why we would suddenly get only half of the frames stored onto disk.

The graph at https://chromeperf.appspot.com/report?sid=c5eba42b2ce75fca5a42ff52776e623756ef164c3adfae970d86d6fb2a95afb2&rev=442081 makes this very clear. 

mcasas: in #20 you're speculating this would have something to do with how we use ffmpeg, can you elaborate? The commands we use are printed in clear in the test logs (and in the code) if you're interested.
Owner: mcasas@chromium.org
mcasas@ are you looking at this?
Ping.
Status: Untriaged (was: Assigned)
Removing myself as owner of these issues are marking them 
for retriaging.
Owner: ----
Cc: phoglund@chromium.org
+phoglund

Patrik, now that you're back: what's your view on this? We still have regressions of
* ~50% for Max_repeated
* ~42% for Unique_frames_count
since January 17 when this and bug 681815 were filed.

It affects all platforms, all codecs and both 360p and 720p.
Certainly we can accept the regression if the discussion above is concluding that it doesn't affect the end-user experience. We're likely able to spot new regressions also with the new (lower) baseline. It just feels wrong to ignore what's going on here.
I guess we either get less unique frames to the <video> tag, or less unique frames in file produced by the media recorder. That sounds like a clear reduction in performance for the media recorder or maybe even for WebRTC calls in general. Surely it must be better to capture more frames for the same duration of video?

I doubt we would a 40% reduction in unique frames would have slipped into major releases unnoticed, but I suppose MediaRecorder could have become worse since it sees less usage. What's a good way to find out, blind tests of recorded videos before and after the CL? Miguel, how did you verify your CL was good and didn't worsen quality?
#33: content_browsertests [1] did not regress, and we didn't see any
bug reports from the field, where it's being used actively, see the
UMA https://uma.googleplex.com/timeline_v2?sid=dfa61b867672d32c08e1b8a93268f50c)

[1] https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_media_recorder_browsertest.cc?sq=package:chromium&dr=CSs
Status: WontFix (was: Untriaged)
Ok, if you are convinced the media recorder is fine, there's not a lot more to do and we should WontFix this. 

The regression makes the tests less sensitive, which is unfortunate, but it's not a big deal. The absolute numbers are not that important, only the changes in them.

Sign in to add a comment