Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1913233002:
Fix MediaRecorder bitrate configuration bug (Closed)
|
Created:
4 years, 8 months ago by jnoring Modified:
4 years, 8 months ago Reviewers:
mcasas CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix MediaRecorder bitrate configuration bug
The MediaRecorder API specifies that bitrates be communicated in bits
per second, which is correctly plumbed through Chrome. However, VPx
encoders expect its arguments to be specified in kilobits per second.
This resulted in specifying any bitrate via the javascript API to be
off by a thousand, resulting in enormous files. Change it so we
convert bits -> kilobits by dividing by 1000
BUG=605750
R=mcasas@chromium.org
Committed: https://crrev.com/0a58e7128c0559027991ae4671fb7b16274b7ba3
Cr-Commit-Position: refs/heads/master@{#389411}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Correct git commit messages #Patch Set 3 : correct a comment #Messages
Total messages: 22 (6 generated)
Please review when you have a moment. Thank you!
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913233002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913233002/1
On 2016/04/22 18:48:54, jnoring wrote: > Please review when you have a moment. Thank you! Looking good, just a nit. Let's if all bots are happy. Also please format the description to ~72 chars per line so is correctly formatted in bundle logs, and replace "VP8 encoder" with "VPx encoder" (since the encoder supports many codecs).
Description was changed from ========== Fix MediaRecorder bitrate configuration bug The MediaRecorder API specifies that bitrates be communicated in bits per second, which is correctly plumbed through Chrome. However, the VP8 encoder expects its arguments to be specified in kilobits per second. This resulted in specifying any bitrate via the javascript API to be off by a thousand, resulting in enormous files. Change it so we convert bits -> kilobits by dividing by 1000 BUG=605750 R=mcasas@chromium.org ========== to ========== Fix MediaRecorder bitrate configuration bug The MediaRecorder API specifies that bitrates be communicated in bits per second, which is correctly plumbed through Chrome. However, VPx encoders expect its arguments to be specified in kilobits per second. This resulted in specifying any bitrate via the javascript API to be off by a thousand, resulting in enormous files. Change it so we convert bits -> kilobits by dividing by 1000 BUG=605750 R=mcasas@chromium.org ==========
On 2016/04/22 20:34:55, mcasas wrote: > Looking good, just a nit. Let's if all bots are happy. > Also please format the description to ~72 chars per > line so is correctly formatted in bundle logs, and > replace "VP8 encoder" with "VPx encoder" (since the > encoder supports many codecs). Updated--thanks! Could you confirm I got this correct?
On 2016/04/22 20:40:15, jnoring wrote: > On 2016/04/22 20:34:55, mcasas wrote: > > Looking good, just a nit. Let's if all bots are happy. > > Also please format the description to ~72 chars per > > line so is correctly formatted in bundle logs, and > > replace "VP8 encoder" with "VPx encoder" (since the > > encoder supports many codecs). > > Updated--thanks! Could you confirm I got this correct? Description ell-gee-tee-em, now upload a new patch with the nit addressed and I'll stamp it.
On 2016/04/22 20:42:47, mcasas wrote: > On 2016/04/22 20:40:15, jnoring wrote: > > On 2016/04/22 20:34:55, mcasas wrote: > > > Looking good, just a nit. Let's if all bots are happy. > > > Also please format the description to ~72 chars per > > > line so is correctly formatted in bundle logs, and > > > replace "VP8 encoder" with "VPx encoder" (since the > > > encoder supports many codecs). > > > > Updated--thanks! Could you confirm I got this correct? > > Description ell-gee-tee-em, now upload a new patch > with the nit addressed and I'll stamp it. I'm sorry--I must be dense. I'm really not following you? Is editing the description not enough?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ooops sorry, I forgot to publish my comments. Please see below. https://codereview.chromium.org/1913233002/diff/1/content/renderer/media/vide... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1913233002/diff/1/content/renderer/media/vide... content/renderer/media/video_track_recorder.cc:295: // actual size. Note: rtc_target_bitrate units are kbit per second. nit: |rtc_target_bitrate| (variables and members are escaped).
Ah, I see now. Okay, I've pushed a correction.
On 2016/04/22 21:52:57, jnoring wrote: > Ah, I see now. Okay, I've pushed a correction. LGTM. Ship it!
On 2016/04/22 22:27:05, mcasas wrote: > On 2016/04/22 21:52:57, jnoring wrote: > > Ah, I see now. Okay, I've pushed a correction. > > LGTM. Ship it! :yey:
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913233002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913233002/40001
On 2016/04/22 22:27:26, jnoring wrote: > On 2016/04/22 22:27:05, mcasas wrote: > > On 2016/04/22 21:52:57, jnoring wrote: > > > Ah, I see now. Okay, I've pushed a correction. > > > > LGTM. Ship it! > > :yey: To commit, you had to click on the "Commit" tick box below the list of files, and that should send it to the commit queue. Just done it for you, so it should be now "in flight".
Message was sent while issue was closed.
Description was changed from ========== Fix MediaRecorder bitrate configuration bug The MediaRecorder API specifies that bitrates be communicated in bits per second, which is correctly plumbed through Chrome. However, VPx encoders expect its arguments to be specified in kilobits per second. This resulted in specifying any bitrate via the javascript API to be off by a thousand, resulting in enormous files. Change it so we convert bits -> kilobits by dividing by 1000 BUG=605750 R=mcasas@chromium.org ========== to ========== Fix MediaRecorder bitrate configuration bug The MediaRecorder API specifies that bitrates be communicated in bits per second, which is correctly plumbed through Chrome. However, VPx encoders expect its arguments to be specified in kilobits per second. This resulted in specifying any bitrate via the javascript API to be off by a thousand, resulting in enormous files. Change it so we convert bits -> kilobits by dividing by 1000 BUG=605750 R=mcasas@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix MediaRecorder bitrate configuration bug The MediaRecorder API specifies that bitrates be communicated in bits per second, which is correctly plumbed through Chrome. However, VPx encoders expect its arguments to be specified in kilobits per second. This resulted in specifying any bitrate via the javascript API to be off by a thousand, resulting in enormous files. Change it so we convert bits -> kilobits by dividing by 1000 BUG=605750 R=mcasas@chromium.org ========== to ========== Fix MediaRecorder bitrate configuration bug The MediaRecorder API specifies that bitrates be communicated in bits per second, which is correctly plumbed through Chrome. However, VPx encoders expect its arguments to be specified in kilobits per second. This resulted in specifying any bitrate via the javascript API to be off by a thousand, resulting in enormous files. Change it so we convert bits -> kilobits by dividing by 1000 BUG=605750 R=mcasas@chromium.org Committed: https://crrev.com/0a58e7128c0559027991ae4671fb7b16274b7ba3 Cr-Commit-Position: refs/heads/master@{#389411} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0a58e7128c0559027991ae4671fb7b16274b7ba3 Cr-Commit-Position: refs/heads/master@{#389411} |
