Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1092813003:
Enable Cyclic Refresh for VP9. (Closed)
DescriptionEnable Cyclic Refresh for VP9.
Cyclic refresh allows the encoder to update already coded sections of
the image at a higher quality.
BUG=134202
Committed: https://crrev.com/8a3333697857a0125e5bb68a10e7ea2f83f35d3e
Cr-Commit-Position: refs/heads/master@{#330265}
Patch Set 1 #
Total comments: 29
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #Messages
Total messages: 23 (5 generated)
aconverse@chromium.org changed reviewers: + wez@chromium.org
IIUC this won't have any real effect until we modify the VideoPump to know to pump ~10 more frames after the last non-empty frame it sees? https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... File remoting/codec/video_encoder_vpx.cc (right): https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:44: const int kVp9AqCyclicRefresh = 3; nit: Suggest these start kVp9AqMode, for consistency with the codec-control enum name (VP9E_SET_AQ_MODE) https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:142: // Set cyclic refresh for lossy encode nit: missing . Suggest "Enable cyclic refresh (aka "top-off") only for lossy encoding." https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:304: VPX_CODEC_OK) { Is there any point calling this if we are already running w/ lossless encode? https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:309: updated_region.AddRect(webrtc::DesktopRect::MakeWH(image_->w, image_->h)); Under what circumstances can the get call fail? Should we just CHECK or NOTREACHED here, instead? https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:532: // Mark active areas updated. nit: This comment doesn't fit this function; we're fetching the active areas, not marking them. :) https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:533: for (int r = 0; r < active_map_height_; ++r) { Suggest r -> y https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:534: int c1; Move this inside the c0 loop? https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:535: for (int c0 = 0; c0 < active_map_height_; c0 = c1 + 1) { Looks like this should be |active_map_width_|? https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:535: for (int c0 = 0; c0 < active_map_height_; c0 = c1 + 1) { Suggest c0 -> x0 https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:536: for (c1 = c0; c1 < active_map_height_; ++c1) { Ditto. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... File remoting/codec/video_encoder_vpx.h (right): https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.h:52: // Updates the active map to revealing areas updated by cyclic refresh. This doesn't seem to update the active map at all; it fetches the active map that the encoder actually used for the frame, combining the caller-supplied active map with the areas updated by cyclic refresh, right? Suggest changing PrepareActiveMap() and UpdateActiveMap() to SetActiveMap[FromRegion](...) and GetActiveMap[ToRegion](...). The *Region forms seem clearer to read, but more verbose, so up to you.
On 2015/04/18 00:17:55, Wez wrote: > IIUC this won't have any real effect until we modify the VideoPump to know to > pump ~10 more frames after the last non-empty frame it sees? > Without the pump it will still update. It will just update at the rate the screen is actually changing, not at the capture rate. E.g. for a text cursor blinking at 1 Hz it will top off both when the cursor disappears and reappears (2 fps).
https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... File remoting/codec/video_encoder_vpx.cc (right): https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:44: const int kVp9AqCyclicRefresh = 3; On 2015/04/18 00:17:54, Wez wrote: > nit: Suggest these start kVp9AqMode, for consistency with the codec-control enum > name (VP9E_SET_AQ_MODE) Done. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:142: // Set cyclic refresh for lossy encode On 2015/04/18 00:17:54, Wez wrote: > nit: missing . > > Suggest "Enable cyclic refresh (aka "top-off") only for lossy encoding." Done. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:304: VPX_CODEC_OK) { On 2015/04/18 00:17:54, Wez wrote: > Is there any point calling this if we are already running w/ lossless encode? Done. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:309: updated_region.AddRect(webrtc::DesktopRect::MakeWH(image_->w, image_->h)); On 2015/04/18 00:17:54, Wez wrote: > Under what circumstances can the get call fail? Should we just CHECK or > NOTREACHED here, instead? I was following the example of SET_ACTIVEMAP. Neither call should fail unless the map size doesn't match the image size. Changed. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:532: // Mark active areas updated. On 2015/04/18 00:17:54, Wez wrote: > nit: This comment doesn't fit this function; we're fetching the active areas, > not marking them. :) We've already fetched the areas. We are marking them in the |updated_region|. This is the inverse of the previous comment "Mark updated areas active." https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:533: for (int r = 0; r < active_map_height_; ++r) { On 2015/04/18 00:17:54, Wez wrote: > Suggest r -> y Done. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:534: int c1; On 2015/04/18 00:17:54, Wez wrote: > Move this inside the c0 loop? This is used in the iteration condition so that we are acting on lines of 16x16 blocks and not individual 16x16 blocks. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:535: for (int c0 = 0; c0 < active_map_height_; c0 = c1 + 1) { On 2015/04/18 00:17:54, Wez wrote: > Suggest c0 -> x0 Done. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:535: for (int c0 = 0; c0 < active_map_height_; c0 = c1 + 1) { On 2015/04/18 00:17:54, Wez wrote: > Looks like this should be |active_map_width_|? Done. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:536: for (c1 = c0; c1 < active_map_height_; ++c1) { On 2015/04/18 00:17:54, Wez wrote: > Ditto. Done. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:537: if (map[c1] == 0) Big oops on this line as well. [not taking row into account]. This whole function never should have worked at all as written. It appears that the client is ignoring the updated region. (Demonstrated by clearing |updated_region| and setting it to 300x300 and seeing changes across a whole desktop.) Was that an intentional change? If so we can remove this whole function. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... File remoting/codec/video_encoder_vpx.h (right): https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.h:52: // Updates the active map to revealing areas updated by cyclic refresh. On 2015/04/18 00:17:55, Wez wrote: > This doesn't seem to update the active map at all; it fetches the active map > that the encoder actually used for the frame, combining the caller-supplied > active map with the areas updated by cyclic refresh, right? > > Suggest changing PrepareActiveMap() and UpdateActiveMap() to > SetActiveMap[FromRegion](...) and GetActiveMap[ToRegion](...). The *Region forms > seem clearer to read, but more verbose, so up to you. Done.
BTW, can you set BUG=134202 in the CL description?
Have you run remoting_unittests and remoting_perftests with this patch applied, BTW? We should work out a way to unit-test the cyclic refresh in VP9 lossy - e.g. by verifying that the PSNR between source & output frames drops to close to zero within ten frames - WDYT? https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... File remoting/codec/video_encoder_vpx.cc (right): https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:309: updated_region.AddRect(webrtc::DesktopRect::MakeWH(image_->w, image_->h)); On 2015/04/20 18:32:39, aconverse wrote: > On 2015/04/18 00:17:54, Wez wrote: > > Under what circumstances can the get call fail? Should we just CHECK or > > NOTREACHED here, instead? > > I was following the example of SET_ACTIVEMAP. Neither call should fail unless > the map size doesn't match the image size. > > Changed. Ah, OK; we should probably have SET_ACTIVEMAP errors be more fatal; not a problem for this CL, though. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:532: // Mark active areas updated. On 2015/04/20 18:32:38, aconverse wrote: > On 2015/04/18 00:17:54, Wez wrote: > > nit: This comment doesn't fit this function; we're fetching the active areas, > > not marking them. :) > > We've already fetched the areas. We are marking them in the |updated_region|. > > This is the inverse of the previous comment "Mark updated areas active." OK; suggest "Add areas marked active to |updated_region|", or just removing the comment entirely, since it's already commented on the function declaration. https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:534: int c1; On 2015/04/20 18:32:38, aconverse wrote: > On 2015/04/18 00:17:54, Wez wrote: > > Move this inside the c0 loop? > > This is used in the iteration condition so that we are acting on lines of 16x16 > blocks and not individual 16x16 blocks. Gotcha; I think it might be more readable to declare x1 inside the for(x0) loop, and initialize it to x0, then do the x0 = x1 + 1 assignment at the end of the loop, after the if(). https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:537: if (map[c1] == 0) On 2015/04/20 18:32:39, aconverse wrote: > Big oops on this line as well. [not taking row into account]. > > This whole function never should have worked at all as written. It appears that > the client is ignoring the updated region. (Demonstrated by clearing > |updated_region| and setting it to 300x300 and seeing changes across a whole > desktop.) Was that an intentional change? If so we can remove this whole > function. Yes and no; no, we can't remove this, because we do want the updated-region to be sent so that the client can optimize its rendering. But yes, the client currently ignores it because we use the pp::VideoDecoder API and render them via Graphics3D/GLES2, so there's no real need to optimize that via the updated-region. You can use the remoting.connectedView.enableDebugRegion(true) option to have the client display the updated region, BTW. https://codereview.chromium.org/1092813003/diff/20001/remoting/codec/video_en... File remoting/codec/video_encoder_vpx.h (right): https://codereview.chromium.org/1092813003/diff/20001/remoting/codec/video_en... remoting/codec/video_encoder_vpx.h:52: // Updates the active map to revealing areas updated by cyclic refresh. Suggest: "Adds areas changed in the most recent frame to |updated_region|. This includes both content changes and areas sharpened by cyclic refresh."
It looks like this CL includes the changes from crrev.com/1079593004 as well? If you set that CL's branch as the up-stream for this one then git cl upload should upload the correct diff, i.e. exclude the bit-rate changes from this CL.
On 2015/04/21 01:32:36, Wez wrote: > It looks like this CL includes the changes from crrev.com/1079593004 as well? > > If you set that CL's branch as the up-stream for this one then git cl upload > should upload the correct diff, i.e. exclude the bit-rate changes from this CL. Looking at the diff I don't see those changes.
On 2015/04/28 00:24:19, aconverse wrote: > On 2015/04/21 01:32:36, Wez wrote: > > It looks like this CL includes the changes from crrev.com/1079593004 as well? > > > > If you set that CL's branch as the up-stream for this one then git cl upload > > should upload the correct diff, i.e. exclude the bit-rate changes from this > CL. > > Looking at the diff I don't see those changes. Yup, not sure what I was seeing when I wrote that... :-/
https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... File remoting/codec/video_encoder_vpx.cc (right): https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:534: int c1; On 2015/04/21 01:29:18, Wez wrote: > On 2015/04/20 18:32:38, aconverse wrote: > > On 2015/04/18 00:17:54, Wez wrote: > > > Move this inside the c0 loop? > > > > This is used in the iteration condition so that we are acting on lines of > 16x16 > > blocks and not individual 16x16 blocks. > > Gotcha; I think it might be more readable to declare x1 inside the for(x0) loop, > and initialize it to x0, then do the x0 = x1 + 1 assignment at the end of the > loop, after the if(). Done. lmk if I misunderstood https://codereview.chromium.org/1092813003/diff/1/remoting/codec/video_encode... remoting/codec/video_encoder_vpx.cc:537: if (map[c1] == 0) On 2015/04/21 01:29:18, Wez wrote: > On 2015/04/20 18:32:39, aconverse wrote: > > Big oops on this line as well. [not taking row into account]. > > > > This whole function never should have worked at all as written. It appears > that > > the client is ignoring the updated region. (Demonstrated by clearing > > |updated_region| and setting it to 300x300 and seeing changes across a whole > > desktop.) Was that an intentional change? If so we can remove this whole > > function. > > Yes and no; no, we can't remove this, because we do want the updated-region to > be sent so that the client can optimize its rendering. But yes, the client > currently ignores it because we use the pp::VideoDecoder API and render them via > Graphics3D/GLES2, so there's no real need to optimize that via the > updated-region. > > You can use the remoting.connectedView.enableDebugRegion(true) option to have > the client display the updated region, BTW. I should give this a try before we move forward. https://codereview.chromium.org/1092813003/diff/20001/remoting/codec/video_en... File remoting/codec/video_encoder_vpx.h (right): https://codereview.chromium.org/1092813003/diff/20001/remoting/codec/video_en... remoting/codec/video_encoder_vpx.h:52: // Updates the active map to revealing areas updated by cyclic refresh. On 2015/04/21 01:29:18, Wez wrote: > Suggest: "Adds areas changed in the most recent frame to |updated_region|. This > includes both content changes and areas sharpened by cyclic refresh." Done.
Performance numbers: I'm seeing a 1.45x speedup on the perftest using this feature. This is counter intuitive. In real world usage I'm seeing about a 0.9x slowdown. Before $ ./out/Release/remoting_perftests --single-process-tests --gtest_filter=*Vp9* Note: Google Test filter = *Vp9* [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from VideoEncoderVpxTest [ RUN ] VideoEncoderVpxTest.MeasureVp9Fps [24540:24540:0428/100031:1013452063499:ERROR:video_encoder_vpx_perftest.cc(53)] 1280x1024(lossy )(I420): 68fps [24540:24540:0428/100033:1013453888261:ERROR:video_encoder_vpx_perftest.cc(53)] 1920x1200(lossy )(I420): 42fps [24540:24540:0428/100034:1013455471717:ERROR:video_encoder_vpx_perftest.cc(53)] 1280x1024(lossy )(I444): 50fps [24540:24540:0428/100037:1013457729072:ERROR:video_encoder_vpx_perftest.cc(53)] 1920x1200(lossy )(I444): 36fps [24540:24540:0428/100038:1013459204668:ERROR:video_encoder_vpx_perftest.cc(53)] 1280x1024(lossless)(I420): 220fps [24540:24540:0428/100040:1013461041700:ERROR:video_encoder_vpx_perftest.cc(53)] 1920x1200(lossless)(I420): 125fps [24540:24540:0428/100042:1013462771108:ERROR:video_encoder_vpx_perftest.cc(53)] 1280x1024(lossless)(I444): 162fps [24540:24540:0428/100044:1013465073893:ERROR:video_encoder_vpx_perftest.cc(53)] 1920x1200(lossless)(I444): 93fps [ OK ] VideoEncoderVpxTest.MeasureVp9Fps (14432 ms) [----------] 1 test from VideoEncoderVpxTest (14432 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (14432 ms total) [ PASSED ] 1 test. $ ./out/Release/remoting_unittests --single-process-tests --gtest_filter=*Vp9* Note: Google Test filter = *Vp9* [==========] Running 14 tests from 2 test cases. [----------] Global test environment set-up. [----------] 10 tests from VideoDecoderVp9Test ... [----------] Global test environment tear-down [==========] 14 tests from 2 test cases ran. (1471 ms total) [ PASSED ] 14 tests. After: $ ./out/Release/remoting_perftests --single-process-tests --gtest_filter=*Vp9* Note: Google Test filter = *Vp9* [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from VideoEncoderVpxTest [ RUN ] VideoEncoderVpxTest.MeasureVp9Fps [25255:25255:0428/100526:1013746869937:ERROR:video_encoder_vpx_perftest.cc(53)] 1280x1024(lossy )(I420): 101fps [25255:25255:0428/100528:1013748688635:ERROR:video_encoder_vpx_perftest.cc(53)] 1920x1200(lossy )(I420): 60fps [25255:25255:0428/100529:1013750307080:ERROR:video_encoder_vpx_perftest.cc(53)] 1280x1024(lossy )(I444): 73fps [25255:25255:0428/100531:1013752483085:ERROR:video_encoder_vpx_perftest.cc(53)] 1920x1200(lossy )(I444): 44fps [25255:25255:0428/100533:1013753961202:ERROR:video_encoder_vpx_perftest.cc(53)] 1280x1024(lossless)(I420): 205fps [25255:25255:0428/100535:1013755804639:ERROR:video_encoder_vpx_perftest.cc(53)] 1920x1200(lossless)(I420): 122fps [25255:25255:0428/100537:1013757515232:ERROR:video_encoder_vpx_perftest.cc(53)] 1280x1024(lossless)(I444): 165fps [25255:25255:0428/100539:1013759759366:ERROR:video_encoder_vpx_perftest.cc(53)] 1920x1200(lossless)(I444): 91fps [ OK ] VideoEncoderVpxTest.MeasureVp9Fps (14269 ms) [----------] 1 test from VideoEncoderVpxTest (14269 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (14269 ms total) [ PASSED ] 1 test. $ ./out/Release/remoting_unittests --single-process-tests --gtest_filter=*Vp9* Note: Google Test filter = *Vp9* [==========] Running 14 tests from 2 test cases. [----------] Global test environment set-up. [----------] 10 tests from VideoDecoderVp9Test ... [----------] Global test environment tear-down [==========] 14 tests from 2 test cases ran. (1322 ms total) [ PASSED ] 14 tests.
On 2015/04/28 17:29:49, aconverse wrote: > Performance numbers: > > I'm seeing a 1.45x speedup on the perftest using this feature. This is counter > intuitive. In real world usage I'm seeing about a 0.9x slowdown. > > Before > > $ ./out/Release/remoting_perftests --single-process-tests --gtest_filter=*Vp9* > Note: Google Test filter = *Vp9* > [==========] Running 1 test from 1 test case. > [----------] Global test environment set-up. > [----------] 1 test from VideoEncoderVpxTest > [ RUN ] VideoEncoderVpxTest.MeasureVp9Fps > [24540:24540:0428/100031:1013452063499:ERROR:video_encoder_vpx_perftest.cc(53)] > 1280x1024(lossy )(I420): 68fps > [24540:24540:0428/100033:1013453888261:ERROR:video_encoder_vpx_perftest.cc(53)] > 1920x1200(lossy )(I420): 42fps > [24540:24540:0428/100034:1013455471717:ERROR:video_encoder_vpx_perftest.cc(53)] > 1280x1024(lossy )(I444): 50fps > [24540:24540:0428/100037:1013457729072:ERROR:video_encoder_vpx_perftest.cc(53)] > 1920x1200(lossy )(I444): 36fps > [24540:24540:0428/100038:1013459204668:ERROR:video_encoder_vpx_perftest.cc(53)] > 1280x1024(lossless)(I420): 220fps > [24540:24540:0428/100040:1013461041700:ERROR:video_encoder_vpx_perftest.cc(53)] > 1920x1200(lossless)(I420): 125fps > [24540:24540:0428/100042:1013462771108:ERROR:video_encoder_vpx_perftest.cc(53)] > 1280x1024(lossless)(I444): 162fps > [24540:24540:0428/100044:1013465073893:ERROR:video_encoder_vpx_perftest.cc(53)] > 1920x1200(lossless)(I444): 93fps > [ OK ] VideoEncoderVpxTest.MeasureVp9Fps (14432 ms) > [----------] 1 test from VideoEncoderVpxTest (14432 ms total) > > [----------] Global test environment tear-down > [==========] 1 test from 1 test case ran. (14432 ms total) > [ PASSED ] 1 test. > $ ./out/Release/remoting_unittests --single-process-tests --gtest_filter=*Vp9* > Note: Google Test filter = *Vp9* > [==========] Running 14 tests from 2 test cases. > [----------] Global test environment set-up. > [----------] 10 tests from VideoDecoderVp9Test > ... > [----------] Global test environment tear-down > [==========] 14 tests from 2 test cases ran. (1471 ms total) > [ PASSED ] 14 tests. > > After: > > $ ./out/Release/remoting_perftests --single-process-tests --gtest_filter=*Vp9* > Note: Google Test filter = *Vp9* > [==========] Running 1 test from 1 test case. > [----------] Global test environment set-up. > [----------] 1 test from VideoEncoderVpxTest > [ RUN ] VideoEncoderVpxTest.MeasureVp9Fps > [25255:25255:0428/100526:1013746869937:ERROR:video_encoder_vpx_perftest.cc(53)] > 1280x1024(lossy )(I420): 101fps > [25255:25255:0428/100528:1013748688635:ERROR:video_encoder_vpx_perftest.cc(53)] > 1920x1200(lossy )(I420): 60fps > [25255:25255:0428/100529:1013750307080:ERROR:video_encoder_vpx_perftest.cc(53)] > 1280x1024(lossy )(I444): 73fps > [25255:25255:0428/100531:1013752483085:ERROR:video_encoder_vpx_perftest.cc(53)] > 1920x1200(lossy )(I444): 44fps > [25255:25255:0428/100533:1013753961202:ERROR:video_encoder_vpx_perftest.cc(53)] > 1280x1024(lossless)(I420): 205fps > [25255:25255:0428/100535:1013755804639:ERROR:video_encoder_vpx_perftest.cc(53)] > 1920x1200(lossless)(I420): 122fps > [25255:25255:0428/100537:1013757515232:ERROR:video_encoder_vpx_perftest.cc(53)] > 1280x1024(lossless)(I444): 165fps > [25255:25255:0428/100539:1013759759366:ERROR:video_encoder_vpx_perftest.cc(53)] > 1920x1200(lossless)(I444): 91fps > [ OK ] VideoEncoderVpxTest.MeasureVp9Fps (14269 ms) > [----------] 1 test from VideoEncoderVpxTest (14269 ms total) > > [----------] Global test environment tear-down > [==========] 1 test from 1 test case ran. (14269 ms total) > [ PASSED ] 1 test. > $ ./out/Release/remoting_unittests --single-process-tests --gtest_filter=*Vp9* > Note: Google Test filter = *Vp9* > [==========] Running 14 tests from 2 test cases. > [----------] Global test environment set-up. > [----------] 10 tests from VideoDecoderVp9Test > ... > [----------] Global test environment tear-down > [==========] 14 tests from 2 test cases ran. (1322 ms total) > [ PASSED ] 14 tests. Those tests are pretty dumb right now, and just repeatedly re-encode a sequence of frames of pseudo-random data - in particular no two consecutive frames are the same so could the top-off/cyclic-refresh be the expensive step in the new implementation?
LGTM w/ nit https://codereview.chromium.org/1092813003/diff/40001/remoting/codec/video_en... File remoting/codec/video_encoder_vpx.cc (right): https://codereview.chromium.org/1092813003/diff/40001/remoting/codec/video_en... remoting/codec/video_encoder_vpx.cc:533: // Mark active areas updated. nit: This comment still seems redundant!
The CQ bit was checked by aconverse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/1092813003/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092813003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/05/06 18:53:08, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Looks like we're missing the libvpx roll on which this patch depends?
The CQ bit was checked by wez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092813003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8a3333697857a0125e5bb68a10e7ea2f83f35d3e Cr-Commit-Position: refs/heads/master@{#330265} |
