Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2183873002:
Use MediaRecorder to capture video for quality test. (Closed)
|
Created:
4 years, 4 months ago by ehmaldonado_chromium Modified:
4 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, phoglund+watch_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, tnakamura+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse MediaRecorder to capture video for quality test.
Retrite the WebRTC video quality test to use the MediaRecorder API
instead of recording from a canvas.
BUG=631458
Committed: https://crrev.com/0204f3d64d8472a25d93312356d2a1c4acbb600e
Cr-Commit-Position: refs/heads/master@{#408633}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 18 (7 generated)
ehmaldonado@chromium.org changed reviewers: + phoglund@chromium.org
Hi, This seems to work. Can you take a look and see if it makes sense? If you come over, I can show you the execution results :)
Description was changed from ========== Use MediaRecorder to capture video for quality test. Retrite the WebRTC video quality test to use the MediaRecorder API instead of recording from a canvas. BUG= ========== to ========== Use MediaRecorder to capture video for quality test. Retrite the WebRTC video quality test to use the MediaRecorder API instead of recording from a canvas. BUG=631458 ==========
Hmm. I need to think a bit more about this. I imagined we would want to keep the old test since it exercises the video+canvas pipeline, but I think we can live without that. The second question is if we're testing too much by pulling in the media recorder subsystem into the test as well. The third question is how much the ffmpeg webm-to-yuv conversion distorts the result. It is very, very tempting to land this as-is though because of its simplicity. Let me think a bit more on it and I'll get back to you tomorrow.
On 2016/07/26 14:22:37, phoglund_chrome wrote: > Hmm. I need to think a bit more about this. I imagined we would want to keep the > old test since it exercises the video+canvas pipeline, but I think we can live > without that. The second question is if we're testing too much by pulling in the > media recorder subsystem into the test as well. The third question is how much > the ffmpeg webm-to-yuv conversion distorts the result. It is very, very tempting > to land this as-is though because of its simplicity. Let me think a bit more on > it and I'll get back to you tomorrow. Friendly ping.
Ok then, go for it. Do three things though: - Notify webrtc-perf-sheriffs@ since this will most likely impact perf numbers for the video quality tests (unique_frames_count, PSNR, SSIM, max_repeated, max_skipped). - Notify webrtc-video and say that the video quality test now runs through the media recorder as well and that there's now an additional ffmpeg step (if they complain this impacts quality, remind them this test is looking for regressions and not absolute numbers anyway). - Update the performance-tests page on the wiki if necessary. lgtm
On 2016/07/29 13:09:51, phoglund_chrome_OOO_sep_05 wrote: > Ok then, go for it. Do three things though: > > - Notify webrtc-perf-sheriffs@ since this will most likely impact perf numbers > for the video quality tests (unique_frames_count, PSNR, SSIM, max_repeated, > max_skipped). > - Notify webrtc-video and say that the video quality test now runs through the > media recorder as well and that there's now an additional ffmpeg step (if they > complain this impacts quality, remind them this test is looking for regressions > and not absolute numbers anyway). > - Update the performance-tests page on the wiki if necessary. > > lgtm How do I notify them?
The CQ bit was checked by ehmaldonado@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use MediaRecorder to capture video for quality test. Retrite the WebRTC video quality test to use the MediaRecorder API instead of recording from a canvas. BUG=631458 ========== to ========== Use MediaRecorder to capture video for quality test. Retrite the WebRTC video quality test to use the MediaRecorder API instead of recording from a canvas. BUG=631458 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Use MediaRecorder to capture video for quality test. Retrite the WebRTC video quality test to use the MediaRecorder API instead of recording from a canvas. BUG=631458 ========== to ========== Use MediaRecorder to capture video for quality test. Retrite the WebRTC video quality test to use the MediaRecorder API instead of recording from a canvas. BUG=631458 Committed: https://crrev.com/0204f3d64d8472a25d93312356d2a1c4acbb600e Cr-Commit-Position: refs/heads/master@{#408633} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/0204f3d64d8472a25d93312356d2a1c4acbb600e Cr-Commit-Position: refs/heads/master@{#408633}
Message was sent while issue was closed.
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2183873002/diff/1/chrome/test/data/webrtc/vid... File chrome/test/data/webrtc/video_extraction.js (right): https://codereview.chromium.org/2183873002/diff/1/chrome/test/data/webrtc/vid... chrome/test/data/webrtc/video_extraction.js:39: var mediaRecorder = new MediaRecorder(getStreamFromElement_(videoTag)); Creating the MediaRecorder like this means that the chosen bitrate is left to the UA, and you rather remove that variable from your test chain. I'd recommend adding here |videoBitsPerSecond| (and perhaps the audio equivalent) [1] and making this a very large number, essentially indicating the VP8 encoder (which is what will be used) to throw as little information away as possible. A possible target video bitrate could be e.g. no_compression_bps = width * height * fps * 4 [1] https://www.w3.org/TR/mediastream-recording/#MediaRecorderOptions
Message was sent while issue was closed.
kjellander@chromium.org changed reviewers: + kjellander@chromium.org
Message was sent while issue was closed.
Posted some comments but I'm going to revert this as performance numbers regressed a bit too much after landing this, example: https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fu... https://codereview.chromium.org/2183873002/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_video_quality_browsertest.cc (left): https://codereview.chromium.org/2183873002/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_video_quality_browsertest.cc:168: bool RunARGBtoI420Converter(int width, I'm quite sure this is the only place we use this tool so we should take the opportunity to delete the tool now, i.e. essentially make sure https://cs.chromium.org/search/?q=rgba_to_i420&sq=package:chromium&type=cs returns 0 results. Great to reduce the complexity of the video quality toolchain! https://codereview.chromium.org/2183873002/diff/1/chrome/test/data/webrtc/vid... File chrome/test/data/webrtc/video_extraction.js (right): https://codereview.chromium.org/2183873002/diff/1/chrome/test/data/webrtc/vid... chrome/test/data/webrtc/video_extraction.js:39: var mediaRecorder = new MediaRecorder(getStreamFromElement_(videoTag)); On 2016/07/29 16:20:34, mcasas wrote: > Creating the MediaRecorder like this means that the > chosen bitrate is left to the UA, and you rather remove > that variable from your test chain. I'd recommend adding > here |videoBitsPerSecond| (and perhaps the audio > equivalent) [1] and making this a very large number, > essentially indicating the VP8 encoder (which is what > will be used) to throw as little information away as > possible. A possible target video bitrate could be e.g. > no_compression_bps = width * height * fps * 4 > > > > [1] https://www.w3.org/TR/mediastream-recording/#MediaRecorderOptions Sounds like a good idea to address this as the compression of the captured video likely affect the video quality numbers. I assume you guys wanted to get this landed before Patrik went on vacation last week?
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2199653002/ by kjellander@chromium.org. The reason for reverting is: Performance numbers regressed a bit too much after landing this, example: https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fu.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
