Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1889763002:
blimp: Reduce WebP encoding time on the engine. (Closed)
|
Created:
4 years, 8 months ago by Khushal Modified:
4 years, 8 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, klobag.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionblimp: Reduce WebP encoding time on the engine.
The WebP Encoding during commit serialization blocks the render thread
for close to 300 ms and significantly adds on to the time taken to push
a frame to the client.
Use the config that ensures the lowest encoding time. This reduced the
encode time for an image on google.com from ~215 ms to ~53ms.
BUG=603353
Committed: https://crrev.com/f2aab581ec1f14450be730478ee715cc084544af
Cr-Commit-Position: refs/heads/master@{#387421}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added comment. #
Total comments: 1
Patch Set 3 : Updated comment #Messages
Total messages: 17 (7 generated)
khushalsagar@chromium.org changed reviewers: + nyquist@chromium.org
Description was changed from ========== blimp: Reduce WebP encoding time on the engine. The WebP Encoding during commit serialization blocks the render thread for close to 300 ms and significantly adds on to the time taken to push a frame to the client. Use the config that ensures the lowest encoding time. BUG=603353 ========== to ========== blimp: Reduce WebP encoding time on the engine. The WebP Encoding during commit serialization blocks the render thread for close to 300 ms and significantly adds on to the time taken to push a frame to the client. Use the config that ensures the lowest encoding time. BUG=603353 ==========
nyquist@chromium.org changed reviewers: + urvang@chromium.org
https://codereview.chromium.org/1889763002/diff/1/blimp/engine/renderer/engin... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1889763002/diff/1/blimp/engine/renderer/engin... blimp/engine/renderer/engine_image_serialization_processor.cc:70: config.method = 0; // quality/speed trade-off (0=fast, 6=slower-better). I discussed this with urvang@ in patch set 5 of https://codereview.chromium.org/1680333004/#ps80001 (blimp/common/compositor/blimp_image_serialization_processor.cc). He suggested sticking to the standards for these unless we have very good reasons for why not. How much does this CL change the time it takes to encode? I guess if we in the future push this off to a different thread, it wouldn't matter as much what the quality is then, so maybe you could add a TODO about investigating this again when we make the whole image serialization and sending async? urvang@: Do you have any thoughts around what would be a fast, but still OK quality method for this?
https://codereview.chromium.org/1889763002/diff/1/blimp/engine/renderer/engin... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1889763002/diff/1/blimp/engine/renderer/engin... blimp/engine/renderer/engine_image_serialization_processor.cc:70: config.method = 0; // quality/speed trade-off (0=fast, 6=slower-better). On 2016/04/14 17:19:30, nyquist wrote: > I discussed this with urvang@ in patch set 5 of > https://codereview.chromium.org/1680333004/#ps80001 > (blimp/common/compositor/blimp_image_serialization_processor.cc). He suggested > sticking to the standards for these unless we have very good reasons for why > not. > > How much does this CL change the time it takes to encode? > I guess if we in the future push this off to a different thread, it wouldn't > matter as much what the quality is then, so maybe you could add a TODO about > investigating this again when we make the whole image serialization and sending > async? > > urvang@: Do you have any thoughts around what would be a fast, but still OK > quality method for this? I understand that this is a temporary solution before you make sending of images not part of the critical path. In this case, looks fine for now. Add TODOs to: - Change the value of 'config.method' later. I think a value of 3 or 4 (default) should work later. - Investigate if it's feasible to enable multithreading inside libwebp aka "config.thread_level = 1"
https://codereview.chromium.org/1889763002/diff/1/blimp/engine/renderer/engin... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1889763002/diff/1/blimp/engine/renderer/engin... blimp/engine/renderer/engine_image_serialization_processor.cc:70: config.method = 0; // quality/speed trade-off (0=fast, 6=slower-better). On 2016/04/14 17:41:16, urvang wrote: > On 2016/04/14 17:19:30, nyquist wrote: > > I discussed this with urvang@ in patch set 5 of > > https://codereview.chromium.org/1680333004/#ps80001 > > (blimp/common/compositor/blimp_image_serialization_processor.cc). He suggested > > sticking to the standards for these unless we have very good reasons for why > > not. > > > > How much does this CL change the time it takes to encode? > > I guess if we in the future push this off to a different thread, it wouldn't > > matter as much what the quality is then, so maybe you could add a TODO about > > investigating this again when we make the whole image serialization and > sending > > async? > > > > urvang@: Do you have any thoughts around what would be a fast, but still OK > > quality method for this? > > I understand that this is a temporary solution before you make sending of images > not part of the critical path. > > In this case, looks fine for now. Add TODOs to: > - Change the value of 'config.method' later. I think a value of 3 or 4 (default) > should work later. > - Investigate if it's feasible to enable multithreading inside libwebp aka > "config.thread_level = 1" Thanks for the suggestion Urvang. I tried using multi-threading by setting the thread_level to 1 but it didn't cause a significant difference, only in the order of a few microseconds. The method change alone is of great help.
lgtm https://codereview.chromium.org/1889763002/diff/20001/blimp/engine/renderer/e... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1889763002/diff/20001/blimp/engine/renderer/e... blimp/engine/renderer/engine_image_serialization_processor.cc:74: // crbug/603643. "crbug.com/603643"
lgtm after adressing urvang's comment. Also; could you update the CL description to show your findings of how much it helped?
Description was changed from ========== blimp: Reduce WebP encoding time on the engine. The WebP Encoding during commit serialization blocks the render thread for close to 300 ms and significantly adds on to the time taken to push a frame to the client. Use the config that ensures the lowest encoding time. BUG=603353 ========== to ========== blimp: Reduce WebP encoding time on the engine. The WebP Encoding during commit serialization blocks the render thread for close to 300 ms and significantly adds on to the time taken to push a frame to the client. Use the config that ensures the lowest encoding time. This reduced the encode time for an image on google.com from ~215 ms to ~53ms. BUG=603353 ==========
On 2016/04/14 19:37:07, nyquist wrote: > lgtm after adressing urvang's comment. Also; could you update the CL description > to show your findings of how much it helped? Done. Thanks!
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, urvang@chromium.org Link to the patchset: https://codereview.chromium.org/1889763002/#ps40001 (title: "Updated comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1889763002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889763002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== blimp: Reduce WebP encoding time on the engine. The WebP Encoding during commit serialization blocks the render thread for close to 300 ms and significantly adds on to the time taken to push a frame to the client. Use the config that ensures the lowest encoding time. This reduced the encode time for an image on google.com from ~215 ms to ~53ms. BUG=603353 ========== to ========== blimp: Reduce WebP encoding time on the engine. The WebP Encoding during commit serialization blocks the render thread for close to 300 ms and significantly adds on to the time taken to push a frame to the client. Use the config that ensures the lowest encoding time. This reduced the encode time for an image on google.com from ~215 ms to ~53ms. BUG=603353 Committed: https://crrev.com/f2aab581ec1f14450be730478ee715cc084544af Cr-Commit-Position: refs/heads/master@{#387421} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f2aab581ec1f14450be730478ee715cc084544af Cr-Commit-Position: refs/heads/master@{#387421} |
