Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2371763003:
Increase Mojo video buffer capacity to 3MB (Closed)
|
Created:
4 years, 2 months ago by servolk Modified:
4 years, 2 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, alokp+watch_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIncrease Mojo video buffer capacity to 3MB
Some YouTube 4K videos have frames larger than 2 megabytes, e.g. video
tdWV2xEyOfE has frame larger than 2MB at 385.886 sec. So increase mojo
video buffer capacity to 3MB.
BUG=internal b/31724921
Committed: https://crrev.com/a4bf53ccc7f59f2c24d0703802d7508c58810600
Cr-Commit-Position: refs/heads/master@{#421270}
Patch Set 1 #
Total comments: 4
Patch Set 2 : nit #Messages
Total messages: 21 (14 generated)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Description was changed from ========== Increase Mojo video buffer capacity to 3MB Some YouTube 4K videos have frames larger than 2 megabytes, e.g. video tdWV2xEyOfE has frame larger than 2MB at 385.886 sec. So increase mojo video buffer capacity to 3MB. BUG=internal b/31724921 ========== to ========== Increase Mojo video buffer capacity to 3MB Some YouTube 4K videos have frames larger than 2 megabytes, e.g. video tdWV2xEyOfE has frame larger than 2MB at 385.886 sec. So increase mojo video buffer capacity to 3MB. BUG=internal b/31724921 ==========
servolk@chromium.org changed reviewers: + alokp@chromium.org, xhwang@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
non-owner lgtm
LGTM with nit/question https://codereview.chromium.org/2371763003/diff/1/media/mojo/common/mojo_deco... File media/mojo/common/mojo_decoder_buffer_converter.cc (right): https://codereview.chromium.org/2371763003/diff/1/media/mojo/common/mojo_deco... media/mojo/common/mojo_decoder_buffer_converter.cc:32: // Video can get quite large; at 4K, VP9 delivers packets which are larger nit: "which are larger than 2MB" sounds like all packets are larger than 2MB. How about something like "which could be larger than .."? https://codereview.chromium.org/2371763003/diff/1/media/mojo/common/mojo_deco... media/mojo/common/mojo_decoder_buffer_converter.cc:35: options.capacity_num_bytes = 3 * (1024 * 1024); When it's 2MB, I wonder whether we will have performance issue copying the data into and out of the message pipe. Do you see any problem there?
https://codereview.chromium.org/2371763003/diff/1/media/mojo/common/mojo_deco... File media/mojo/common/mojo_decoder_buffer_converter.cc (right): https://codereview.chromium.org/2371763003/diff/1/media/mojo/common/mojo_deco... media/mojo/common/mojo_decoder_buffer_converter.cc:32: // Video can get quite large; at 4K, VP9 delivers packets which are larger On 2016/09/27 16:38:12, xhwang (slow) wrote: > nit: "which are larger than 2MB" sounds like all packets are larger than 2MB. > How about something like "which could be larger than .."? Done. https://codereview.chromium.org/2371763003/diff/1/media/mojo/common/mojo_deco... media/mojo/common/mojo_decoder_buffer_converter.cc:35: options.capacity_num_bytes = 3 * (1024 * 1024); On 2016/09/27 16:38:12, xhwang (slow) wrote: > When it's 2MB, I wonder whether we will have performance issue copying the data > into and out of the message pipe. Do you see any problem there? AFAIR IPC performance has never been a performance bottleneck for us. The bottleneck for media streaming is typically network bandwidth and IPC performance is typically orders of magnitude faster. So I'm not concerned about performance here. What I'm concerned about is that perhaps even 3MB could be not enough for some high-bitrate videos, so perhaps a better fix would be to allow partial buffer transfers through IPC rather than blindly hardcoding the capacity. But that would be a much riskier fix, which we probably wouldn't take into Chromecast until we get a chance to bake/test it for some time.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, alokp@chromium.org Link to the patchset: https://codereview.chromium.org/2371763003/#ps20001 (title: "nit")
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 ========== Increase Mojo video buffer capacity to 3MB Some YouTube 4K videos have frames larger than 2 megabytes, e.g. video tdWV2xEyOfE has frame larger than 2MB at 385.886 sec. So increase mojo video buffer capacity to 3MB. BUG=internal b/31724921 ========== to ========== Increase Mojo video buffer capacity to 3MB Some YouTube 4K videos have frames larger than 2 megabytes, e.g. video tdWV2xEyOfE has frame larger than 2MB at 385.886 sec. So increase mojo video buffer capacity to 3MB. BUG=internal b/31724921 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Increase Mojo video buffer capacity to 3MB Some YouTube 4K videos have frames larger than 2 megabytes, e.g. video tdWV2xEyOfE has frame larger than 2MB at 385.886 sec. So increase mojo video buffer capacity to 3MB. BUG=internal b/31724921 ========== to ========== Increase Mojo video buffer capacity to 3MB Some YouTube 4K videos have frames larger than 2 megabytes, e.g. video tdWV2xEyOfE has frame larger than 2MB at 385.886 sec. So increase mojo video buffer capacity to 3MB. BUG=internal b/31724921 Committed: https://crrev.com/a4bf53ccc7f59f2c24d0703802d7508c58810600 Cr-Commit-Position: refs/heads/master@{#421270} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a4bf53ccc7f59f2c24d0703802d7508c58810600 Cr-Commit-Position: refs/heads/master@{#421270} |
