CrGpuMain spending a lot of time on VAVDA::OutputSurface |
|||||||||||||||||||||||
Issue descriptionOn ToT samus, playing http://crosvideo.appspot.com/?codec=vp9&resolution=2160 we noticed CrGpuMain spends a lot of time on VAVDA::OutputSurface. Sometimes VAVDA::OutputSurface takes up to 30ms, preventing GbmSurfaceless::SwapBufferAsync to execute, thus causing frame drops for all the system and not just the video. Changing the codec from vp9 to vp8 (I'm guessing it switches to SW decoder) seems to alleviate the issue, and the system seems to drop less frames for the UI and scrolling. Link to vp8 for convenience: playing http://crosvideo.appspot.com/?codec=vp8&resolution=2160 ⛆ |
|
|
,
May 3 2017
I think it is happening specifically on platforms which use the hybrid vp9 video decoder. I wouldn't be surprised, since the code isn't particularly efficient. I don't *think* libva as a whole will exhibit this issue. Could you look at a "real" libva decoding situation and see if it suffers the same issue? Maybe try to see if H264 exhibits the issue, and go from there? If the hybrid decoder is the root cause, we could disable it if needed.
,
May 3 2017
http://crosvideo.appspot.com/?codec=h264&resolution=2160 seems much better than vp9. Scrolling is as smooth as with vp8. Looking at a trace I don't see anything at all happening on the VaapiDecoderThread though, so it might just be doing SW decoding.
,
May 3 2017
@3: did you make an internal chrome build with mediacodec etc. enabled?
,
May 3 2017
I was under the impression h.264 wouldn't decode at all without all the appropriate flags. Btw, just checked again after I flashed samus/R60-9517.0.0/test. It claims to be using GpuVideoDecoder and it's smooth. I see some work on the VaapiDecoderThread but I don't see any VAVDA::OutputSurface call.
,
May 5 2017
,
Jun 26 2017
,
Jun 26 2017
,
Jun 26 2017
,
Jun 27 2017
,
Jun 27 2017
,
Jun 27 2017
This could be a potential candidate to reduce jank + stutter in Android games while background decoding processes are going on, so bumped it to P1 and put it into the next milestones.
,
Oct 27 2017
,
Oct 27 2017
I'm confused about this bug triaging :-) - It's marked P1, but for M-60/M-61, which is already gone. - It's marked "Available" but it has an Owner - It's marked OS>Kernel>Video but has nothing to do with the Kernel
,
Oct 27 2017
At this point, we should just get rid of the hybrid decoder entirely and close.
,
Oct 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aed1b2a9f98eb934807b10e7c69f99069da8e70f commit aed1b2a9f98eb934807b10e7c69f99069da8e70f Author: Miguel Casas-Sanchez <mcasas@chromium.org> Date: Tue Oct 31 13:38:51 2017 vaapi: use |sequence_checker_| in vaapi_*_picture.cc This CL makes |sequence_checker_| a protected member var in vaapi_picture.h and uses it in every method in the derived classes in vaapi_{drm,tfp}_picture.cc. Opportunistically it also moves the service function BufferFormatToInternalFormat() to an anonymous namespace in vaapi_drm_picture.cc. TEST=**No new code intended**, these classes are supposed to be single threaded, so this is just a help for future and present devs by enforcing assumptions. (But just in case I tried the usual playback in Soraka-simplechrome, ensuring dcheck_always_on = true in the appropriate args.gn, otherwise Release builds don't have DCHECKs on). Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I9e96440ec7ade318f5ac4c9289fcee1e77c6d90a Reviewed-on: https://chromium-review.googlesource.com/744903 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Pawel Osciak <posciak@chromium.org> Cr-Commit-Position: refs/heads/master@{#512819} [modify] https://crrev.com/aed1b2a9f98eb934807b10e7c69f99069da8e70f/media/gpu/vaapi_drm_picture.cc [modify] https://crrev.com/aed1b2a9f98eb934807b10e7c69f99069da8e70f/media/gpu/vaapi_picture.h [modify] https://crrev.com/aed1b2a9f98eb934807b10e7c69f99069da8e70f/media/gpu/vaapi_tfp_picture.cc
,
Oct 31 2017
Got two options wired to move the blit out of GPU main thread: - move it to a new blit thread https://crrev.com/c/742287 - move it to the decoder thread https://crrev.com/c/747447 Following up on my comments on #14, assigning to me etc.
,
Oct 31 2017
I'd go with that solution (crrev.com/c/747447) if we don't have any regression from delaying buffers/videoframe processing because of the additional blitting. The additional thread (per video) is not going scale well when we have dozens of video playbacks.
,
Nov 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d30df3950287b6b49ffefccc76fb832f37a710c5 commit d30df3950287b6b49ffefccc76fb832f37a710c5 Author: Miguel Casas-Sanchez <mcasas@chromium.org> Date: Wed Nov 01 12:13:39 2017 vaapi: minor style cleanup of inner classes This CL updates code in a few locations in vaapi_decode_accelerator.cc: - inlines the ctor and emtpy destructor of Vaapi{H264,VP8,VP9}Picture because, since they're in a .cc file anyway, separating declaration and definition is unnecessary. - makes some members const in VaapiDecodeSurface. - makes ctors explicit and uses scoped_refptr (and not const&), see [1] and the discussion in [2]: "If the function (at least sometimes) takes a ref on a refcounted object, declare the param as scoped_refptr<T>. The caller can decide whether it wishes to transfer ownership (by calling std::move(t) when passing t) or retain its ref (by simply passing t directly)." "A great deal of Chromium code predates the above rules. In particular, some functions take ownership of params passed as T*, or take const scoped_refptr<T>& instead of T*, [...]. Try to clean up such code when you find it" [1] https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions [2] https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/TlL1D-Djta0 TEST= **No new code intended**. Compiled simplechrome and loaded on Soraka, then played videos making sure (via chrome://media-internals) that GpuVideoDecoder was used. Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I6006fb23325d23841c8613fa41d65f8707a5446c Reviewed-on: https://chromium-review.googlesource.com/746369 Reviewed-by: Kuang-che Wu <kcwu@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#513108} [modify] https://crrev.com/d30df3950287b6b49ffefccc76fb832f37a710c5/media/gpu/vaapi_video_decode_accelerator.cc
,
Nov 2 2017
Re. the patch under review in https://crrev.com/c/747447 ("move it to the decoder thread"), while following posciak@s instruction to further test/perfcheck in [1] I found a strange race condition that shows up eventually, and that boils down to a variation of the dining philosophers problem in disguise: to produce an output video frame, both a VAsurface and an output buffer are required, the latter is returned by the client whereas the former is released by the destructor of VASurface, which is tied up in a callback somewhere. The decoder thread is meanwhile Wait()ing for these two resources to be available, however ATM only the dtor can Signal() it. This is not obvious in ToT code because it all happens on the gpu main task runner, but when moving things to the decoder thread, it becomes an issue. For more fun, there's another ConditionVariable that the decoder thread can be stuck at, waiting for input encoded buffers ("bitstreams"). Conclusion: more refactoring is needed before landing the said CL. Alternatively, the " move it to a new blit thread" CL (https://crrev.com/c/742287) works and passes unittests and shows no performance regression. People in CC:, WDYT? Should we review and land the "new blit thread" CL and work on the necessary cleanups (maybe adding unittests?) towards using a single decoding thread, or just do the necessary prep work and then move the code to the decoder thread? [1] https://chromium-review.googlesource.com/c/chromium/src/+/747447#message-e4ace381bbeee341628b69b8e272c8eb0053770
,
Nov 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e444ab3f98c64627af574d4675e835070e004bc9 commit e444ab3f98c64627af574d4675e835070e004bc9 Author: Miguel Casas-Sanchez <mcasas@chromium.org> Date: Fri Nov 03 17:52:11 2017 vaapi: correct TRACE_EVENTs in VaapiVideoDecodeAccelerator This CL corrects 2 TRACE_EVENTs: - DecodeTask() has a TRACE_EVENT that just measures the lifetime of DecodeTask(), which is likely not the intention, since as long as there is |input_buffers_| and Decode() succeeds, it'll keep on running (see traces). Instead, this CL measures the time spent in decoder_->Decode(). See e.g. ToT [1] versus with this patch [2]. - OutputPicture() has a TRACE_EVENT (called OutputSurface) measuring DownloadFromSurface() _and_ client_->PictureReady(); the name is unrelated to either calls and measuring PictureReady is likely unintended. This CL reduces the scope to only include DownloadFromSurface and renames the tracing. TEST=compile a simplechrome and send to Soraka, find a page with a e.g. VP9 video and record a chrome:tracing with "Video Recorder" category enabled. [1] https://i.imgur.com/YoIi6Pf.png (or https://imgur.com/a/NNGp2). [2] https://i.imgur.com/RoZsLrM.png (or https://imgur.com/a/TWU35). Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I8ba7fd227defea46d0597c7df9fddb0549e51f53 Reviewed-on: https://chromium-review.googlesource.com/746540 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Cr-Commit-Position: refs/heads/master@{#513833} [modify] https://crrev.com/e444ab3f98c64627af574d4675e835070e004bc9/media/gpu/vaapi_video_decode_accelerator.cc
,
Nov 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/359003147f86e312af0fa6b4121dfdd35714eaa1 commit 359003147f86e312af0fa6b4121dfdd35714eaa1 Author: Miguel Casas-Sanchez <mcasas@chromium.org> Date: Fri Nov 03 18:39:45 2017 vaapi: make struct InputBuffer (more) internal This CL makes InputBuffer a class, cleans up method names and constness and adds a constructor. It also brings the member vars related to it together, with the idea that in the near future the behaviour or unbounded-blocking-queue can be somehow encapsulated and/or refactored. TEST=compile simplechrome and run in soraka with [1] making sure GpuVideoDecoder is used in chrome://media-internals. Also compile-deploy-run video_decode_accelerator_unittests and observe no changes in the values spit by the test cases. [1] crosvideo2.appspot.com Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I3e08fc4667c20ff3ed580b6564f43a4786642f98 Reviewed-on: https://chromium-review.googlesource.com/751442 Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Kuang-che Wu <kcwu@chromium.org> Commit-Queue: Daniele Castagna <dcastagna@chromium.org> Cr-Commit-Position: refs/heads/master@{#513853} [modify] https://crrev.com/359003147f86e312af0fa6b4121dfdd35714eaa1/media/gpu/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/359003147f86e312af0fa6b4121dfdd35714eaa1/media/gpu/vaapi_video_decode_accelerator.h
,
Nov 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1a39e48a6d42a6fdbd6304b492dd90e28ac49a59 commit 1a39e48a6d42a6fdbd6304b492dd90e28ac49a59 Author: Miguel Casas-Sanchez <mcasas@chromium.org> Date: Tue Nov 07 03:28:42 2017 vaapi: move vaapi_{,drm,tfp}_picture.{cc,h} to vaapi/ folder This CL moves the said files vaapi_{,drm,tfp}_picture.{cc,h} from //media/gpu to //media/gpu/vaapi. Used tools/git/mass_rename.py TEST=**no new code**, if it compiles, we're good. Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ibcc4e4d5de54c255cf7959496e358cf440ed1491 Reviewed-on: https://chromium-review.googlesource.com/750023 Reviewed-by: Dan Sanders <sandersd@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#514374} [modify] https://crrev.com/1a39e48a6d42a6fdbd6304b492dd90e28ac49a59/media/gpu/BUILD.gn [rename] https://crrev.com/1a39e48a6d42a6fdbd6304b492dd90e28ac49a59/media/gpu/vaapi/vaapi_drm_picture.cc [rename] https://crrev.com/1a39e48a6d42a6fdbd6304b492dd90e28ac49a59/media/gpu/vaapi/vaapi_drm_picture.h [rename] https://crrev.com/1a39e48a6d42a6fdbd6304b492dd90e28ac49a59/media/gpu/vaapi/vaapi_picture.cc [rename] https://crrev.com/1a39e48a6d42a6fdbd6304b492dd90e28ac49a59/media/gpu/vaapi/vaapi_picture.h [rename] https://crrev.com/1a39e48a6d42a6fdbd6304b492dd90e28ac49a59/media/gpu/vaapi/vaapi_tfp_picture.cc [rename] https://crrev.com/1a39e48a6d42a6fdbd6304b492dd90e28ac49a59/media/gpu/vaapi/vaapi_tfp_picture.h [modify] https://crrev.com/1a39e48a6d42a6fdbd6304b492dd90e28ac49a59/media/gpu/vaapi_jpeg_decode_accelerator.cc [modify] https://crrev.com/1a39e48a6d42a6fdbd6304b492dd90e28ac49a59/media/gpu/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/1a39e48a6d42a6fdbd6304b492dd90e28ac49a59/media/gpu/vaapi_wrapper.cc
,
Nov 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8bfb33e869411269f79fc436b55f133380b8487f commit 8bfb33e869411269f79fc436b55f133380b8487f Author: Miguel Casas-Sanchez <mcasas@chromium.org> Date: Fri Nov 10 06:24:26 2017 vaapi: start adding vaapi_video_decode_accelerator_unittest This CL adds a new file vaapi_video_decode_accelerator_unittest and adds it to media_unittests when use_vaapi is true. The new file is then compiled as part of the top level chromiumos_preflight group (in particular by chromeos_amd64-generic_chromium_compile_only_ng), and also in the linux_chromium_chromeos_ bots. This CL also adds a few test cases, dealing mostly with the interaction of input buffers and the VaVDA's internal |decoder_|. (More tests to come). Note that: vaapi_..._unittest needs vaapi to be compiled but not to be run. Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I4b8768e79401170269190292ebf18ef616adb1f8 Reviewed-on: https://chromium-review.googlesource.com/753756 Commit-Queue: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> Cr-Commit-Position: refs/heads/master@{#515477} [modify] https://crrev.com/8bfb33e869411269f79fc436b55f133380b8487f/media/gpu/BUILD.gn [modify] https://crrev.com/8bfb33e869411269f79fc436b55f133380b8487f/media/gpu/vaapi_video_decode_accelerator.h [add] https://crrev.com/8bfb33e869411269f79fc436b55f133380b8487f/media/gpu/vaapi_video_decode_accelerator_unittest.cc
,
Nov 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cec561000e100ca4cb94886fe2605996ccba2b38 commit cec561000e100ca4cb94886fe2605996ccba2b38 Author: Miguel Casas-Sanchez <mcasas@chromium.org> Date: Mon Nov 13 01:31:22 2017 vaapi: cleanup order of dvlog-dchecks-code This CL cleans up a bit the order of DLOG()s and DCHECK()s, so that debug printouts are before DCHECK()s, and DCHECK()s of the current task runner go before the actual code. This CL also removes the function name from VLOGF()s because those already include it, so it's just duplicated. Also I shortened a condition if (a) return false; return true; into: return !a; TEST=No new code (except the condition simplification). Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I627b3077904a87ecd0da8f0cfbfaa7d70d232c35 Reviewed-on: https://chromium-review.googlesource.com/765109 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Pawel Osciak <posciak@chromium.org> Cr-Commit-Position: refs/heads/master@{#515874} [modify] https://crrev.com/cec561000e100ca4cb94886fe2605996ccba2b38/media/gpu/vaapi_video_decode_accelerator.cc
,
Nov 13 2017
,
Nov 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/694365f731cd3706645514497f48c9f5da87cd33 commit 694365f731cd3706645514497f48c9f5da87cd33 Author: Miguel Casas-Sanchez <mcasas@chromium.org> Date: Wed Nov 15 17:53:08 2017 vaapi: cleanup: move some methods to unnamed namespace This CL does some minor cleanups in vaapi_wrapper: - moves ProfileToVAProfile from class-method to internal unnamed namespace, verbatim (except where VaapiWrapper:: prefix was needed to unambinguate symbols). - moves DestroyVAImage(), file-static, to unnamed namespace - moves LazyProfileInfos::GetSupportedProfileInfosForCodecModeInternal() to unnamed namespace (forgotten in crrev.com/c/769989). TEST=**no new code intended** Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I1f203a95f8fb9cf7262391ec0423ef0f0e983ded Reviewed-on: https://chromium-review.googlesource.com/771776 Reviewed-by: Kuang-che Wu <kcwu@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#516739} [modify] https://crrev.com/694365f731cd3706645514497f48c9f5da87cd33/media/gpu/vaapi_wrapper.cc [modify] https://crrev.com/694365f731cd3706645514497f48c9f5da87cd33/media/gpu/vaapi_wrapper.h
,
Nov 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7e372033501de49e5544b4e75e19e36f5d6614af commit 7e372033501de49e5544b4e75e19e36f5d6614af Author: Miguel Casas-Sanchez <mcasas@chromium.org> Date: Wed Nov 15 19:26:20 2017 vaapi: add more testing to vaapi decoder unittests This Cl beefs up a vaapi_video_decode_accelerator_unittests test case. For that we need to mock VaapiWrapper, so it's made overrideable and two of its methods virtual. This CL also turns the static call to VaapiPicture's CreatePicture() into a Callback so it can be overriden by the test. (This is temporary: crbug.com/784507 ). MockVaapiPicture has to be MEDIA_GPU_EXPORTed to link against in the unittests (different link unit). This is important to test the usual decode startup sequence with allocation of PictureBuffers and VASurfaces: test VAVDA mock_decoder_ mock_wrapper_ QueueInputBuffer()---> Decode()--> <I need PictureBuffers> ProvidePicture <---------------- Buffers() -----------> Assign Picture Buffers()----------------> CreateSurfaces Decode()--> <ok, done!> NotifyEndOf <------ BitstreamBuffer() Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I716a54c98b44d28d6405f923f833a893c6d89a40 Reviewed-on: https://chromium-review.googlesource.com/763920 Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#516780} [modify] https://crrev.com/7e372033501de49e5544b4e75e19e36f5d6614af/media/gpu/vaapi/vaapi_picture.h [modify] https://crrev.com/7e372033501de49e5544b4e75e19e36f5d6614af/media/gpu/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/7e372033501de49e5544b4e75e19e36f5d6614af/media/gpu/vaapi_video_decode_accelerator.h [modify] https://crrev.com/7e372033501de49e5544b4e75e19e36f5d6614af/media/gpu/vaapi_video_decode_accelerator_unittest.cc [modify] https://crrev.com/7e372033501de49e5544b4e75e19e36f5d6614af/media/gpu/vaapi_wrapper.h
,
Nov 15 2017
,
Nov 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50a29f826dd31b042ef5f8035962376c2406ff26 commit 50a29f826dd31b042ef5f8035962376c2406ff26 Author: Miguel Casas-Sanchez <mcasas@chromium.org> Date: Thu Nov 16 01:22:05 2017 vavda: add release cb to InputBuffer This CL adds a |release_cb_| member to the InputBuffer internal structure, and binds it where appropriate to the |client_|'s Client::NotifyEndOfBitstreamBuffer. This way, the InputBuffer cleans after itself, the lifetime is easier to follow and the code is less populated. Also took the chance to rename (and make size_t) s/num_stream_bufs_at_decoder_/num_input_buffers_/ because it's a related cleanup. TEST=Simplechrome and video_decode_accelerator_unittests on Soraka. No unittests performance numbers regression observed. Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ia3d35e609ef06141a840ed200f842af285a9b6f0 Reviewed-on: https://chromium-review.googlesource.com/773001 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Pawel Osciak <posciak@chromium.org> Cr-Commit-Position: refs/heads/master@{#516938} [modify] https://crrev.com/50a29f826dd31b042ef5f8035962376c2406ff26/media/gpu/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/50a29f826dd31b042ef5f8035962376c2406ff26/media/gpu/vaapi_video_decode_accelerator.h
,
Nov 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/db60e6825b2fd3832b36a959e71a33005632e218 commit db60e6825b2fd3832b36a959e71a33005632e218 Author: Miguel Casas-Sanchez <mcasas@chromium.org> Date: Thu Nov 16 19:17:50 2017 vaapi: micro cleanups VaapiVideoDecodeAccelerator This CL does some micro cleanups in VaapiVideoDecodeAccelerator: - removes the member |num_input_buffers_|, which is use for TRACEing only, and uses instead |input_buffers.size()|. This is a change in behaviour, since now we would also count the Flush-type InputBuffers, but I think the change is justified since it's just for TRACEing and they are, after all, InputBuffers as well. With the same logic s/Stream buffers at decoder/Input buffers/ at the TRACE_COUNTER1 statements. - Rewrites the logic in GetInputBuffer_Locked() to avoid deep condition layers, using instead early return. - Rewrites Reset() to have less lines. - Removes one of two consecutive empty lines. - Simplifies CreateSurface() return statement. Except for the |num_input_buffers_|, **there should be no new code**. Nonetheless: TEST=compiled-ran video_decode_accelerator_unittests on soraka, all passing w/ same perf numbers. Ran standard VP9 playback, all good as on ToT. Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Id96d63f3245c04008927f94daf3ae05d48be9bbd Reviewed-on: https://chromium-review.googlesource.com/774998 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Cr-Commit-Position: refs/heads/master@{#517147} [modify] https://crrev.com/db60e6825b2fd3832b36a959e71a33005632e218/media/gpu/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/db60e6825b2fd3832b36a959e71a33005632e218/media/gpu/vaapi_video_decode_accelerator.h
,
Nov 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c153d786cfd11fddd3fb6c780eb3e55a874589b9 commit c153d786cfd11fddd3fb6c780eb3e55a874589b9 Author: Miguel Casas <mcasas@chromium.org> Date: Tue Nov 21 04:33:51 2017 vavda use InputBuffer and VaapiPicture as unique_ptr This CL teaches vaapi files to treat InputBuffers and VaapiPictures as move-only unique_ptr, and not as linked_ptr: the latter is been deprecated now that we have move-only support in STL containers [1]. Only actual code change is in VaVDA::AssignPictureBuffers() where picture->Allocate() is moved to before pictures_.insert(..std::move(picture)...). [1] https://cs.chromium.org/chromium/src/base/memory/linked_ptr.h?type=cs&sq=package:chromium&l=72 Bug: 717265, 556939 TEST=compiled-run chrome and vda unittests [2] on soraka. Things working as on ToT, no regression in the produced values observed. [2] /tmp/video_decode_accelerator_unittest --test_video_data=test-25fps.vp9:320:240:250:250:35:150:12 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I5e2840a22ec961a437405f43d83207301d5a95bc Reviewed-on: https://chromium-review.googlesource.com/775815 Reviewed-by: Kuang-che Wu <kcwu@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#518113} [modify] https://crrev.com/c153d786cfd11fddd3fb6c780eb3e55a874589b9/media/gpu/vaapi/vaapi_drm_picture.cc [modify] https://crrev.com/c153d786cfd11fddd3fb6c780eb3e55a874589b9/media/gpu/vaapi/vaapi_picture.h [modify] https://crrev.com/c153d786cfd11fddd3fb6c780eb3e55a874589b9/media/gpu/vaapi/vaapi_tfp_picture.cc [modify] https://crrev.com/c153d786cfd11fddd3fb6c780eb3e55a874589b9/media/gpu/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/c153d786cfd11fddd3fb6c780eb3e55a874589b9/media/gpu/vaapi_video_decode_accelerator.h [modify] https://crrev.com/c153d786cfd11fddd3fb6c780eb3e55a874589b9/media/gpu/vaapi_video_decode_accelerator_unittest.cc
,
Nov 28 2017
,
Feb 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/26eb905d94ceb4b0c3a7d42451897dc27e4afaf8 commit 26eb905d94ceb4b0c3a7d42451897dc27e4afaf8 Author: Miguel Casas <mcasas@chromium.org> Date: Fri Feb 16 04:36:35 2018 vaapi deco cleanup: extract VaapiDecodeSurface into its own file This CL extracts VaapiDecodeSurface out of VaapiVideoDecodeAccelerator into a file on its own, because vaapi_video_decode_accelerator.cc is quite large at 1902 lines. It also introduces =default and adds DISALLOW_COPY_AND_ASSIGN to the extracted class. Otherwise no new code, just moving things around. There's also a side bonus: VaapiVideoDecodeAccelerator::VaapiDecodeSurface uses are shortened in vaapi_video_decode_accelerator. Test: Compile and run in simplechrome on soraka. Ran vp8, vp9 videos of crosvideo.appspot.com, verified GpuVideoDecoder engaged. Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ibd7c88357b2f993a5d72e45f65dfff5d67c4194a Reviewed-on: https://chromium-review.googlesource.com/922981 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Emircan Uysaler <emircan@chromium.org> Cr-Commit-Position: refs/heads/master@{#537201} [modify] https://crrev.com/26eb905d94ceb4b0c3a7d42451897dc27e4afaf8/media/gpu/vaapi/BUILD.gn [add] https://crrev.com/26eb905d94ceb4b0c3a7d42451897dc27e4afaf8/media/gpu/vaapi/vaapi_decode_surface.cc [add] https://crrev.com/26eb905d94ceb4b0c3a7d42451897dc27e4afaf8/media/gpu/vaapi/vaapi_decode_surface.h [modify] https://crrev.com/26eb905d94ceb4b0c3a7d42451897dc27e4afaf8/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/26eb905d94ceb4b0c3a7d42451897dc27e4afaf8/media/gpu/vaapi/vaapi_video_decode_accelerator.h
,
Feb 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9dd0a7c17a09ac1df6b5c4246e7a8d1c8aef4760 commit 9dd0a7c17a09ac1df6b5c4246e7a8d1c8aef4760 Author: Miguel Casas <mcasas@chromium.org> Date: Tue Feb 20 17:00:54 2018 vaapi deco cleanup: extract H264, VP8 and VP9 accelerators vaapi_video_decode_accelerator.* have three inner classes for H264, VP8 and VP9 decoding. To reduce the size of the file(s), this CL extracts those into their own files. No new code is introduced in those new files, but VideoDecodeAccelerator needs to make 3 methods public in order for those not-anymore-internal classes to work (idea for next CL would be to make those into an interface that VaapiVDA implements). Test: Compile and run in simplechrome on soraka. Ran h264, vp8, vp9 videos of crosvideo.appspot.com, verified GpuVideoDecoder engaged. Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I054b39a330f3876031fd30d6e3e36878322d9036 Reviewed-on: https://chromium-review.googlesource.com/922811 Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#537800} [modify] https://crrev.com/9dd0a7c17a09ac1df6b5c4246e7a8d1c8aef4760/media/gpu/vaapi/BUILD.gn [add] https://crrev.com/9dd0a7c17a09ac1df6b5c4246e7a8d1c8aef4760/media/gpu/vaapi/vaapi_h264_accelerator.cc [add] https://crrev.com/9dd0a7c17a09ac1df6b5c4246e7a8d1c8aef4760/media/gpu/vaapi/vaapi_h264_accelerator.h [modify] https://crrev.com/9dd0a7c17a09ac1df6b5c4246e7a8d1c8aef4760/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/9dd0a7c17a09ac1df6b5c4246e7a8d1c8aef4760/media/gpu/vaapi/vaapi_video_decode_accelerator.h [add] https://crrev.com/9dd0a7c17a09ac1df6b5c4246e7a8d1c8aef4760/media/gpu/vaapi/vaapi_vp8_accelerator.cc [add] https://crrev.com/9dd0a7c17a09ac1df6b5c4246e7a8d1c8aef4760/media/gpu/vaapi/vaapi_vp8_accelerator.h [add] https://crrev.com/9dd0a7c17a09ac1df6b5c4246e7a8d1c8aef4760/media/gpu/vaapi/vaapi_vp9_accelerator.cc [add] https://crrev.com/9dd0a7c17a09ac1df6b5c4246e7a8d1c8aef4760/media/gpu/vaapi/vaapi_vp9_accelerator.h
,
Feb 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2d5ecad817b6ab0e2676c86c35e206889108963c commit 2d5ecad817b6ab0e2676c86c35e206889108963c Author: Miguel Casas <mcasas@chromium.org> Date: Thu Feb 22 19:03:05 2018 VideoDecodeAccelerator: make Accelerator owned by Decoder {V4L2,Vaapi}VideoDecodeAccelerator classes own both a *Decoder and a number of *Accelerators; the *Decoder in turn gets a naked pointer to the appropriate *Accelerator. This creates an unnecessary complication in the ownership diagram. This CL cleans that up by giving the ownership of the *Accelerator to the appropriate *Decoder, reducing the footprint of the *VideoDecodeAccelerator and with it its internal state. As a side bonus, *Decoder can turn the *Accelerator into a pseudo invariant. After PS9, the naked ptr to VaapiWrapper inside Vaapi*Accelerator is changed to a const scoped_refptr<VaapiWrapper>. * in the former paragraphs is either {H264,VP8,VP9} To simplify the review, I made a diagram of how it looks before this CL: https://goo.gl/EorgA3 (parts changing in red) after this CL: https://goo.gl/UUvXcS Test: simplechrome on soraka and scarlet, playback: h264, vp8, vp9 from crosvideo, then video_decode_accelerator_unittests. Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ic8f1f200a38c2d04b22e1c11e9fcaad61fea575f Reviewed-on: https://chromium-review.googlesource.com/928826 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Pawel Osciak <posciak@chromium.org> Cr-Commit-Position: refs/heads/master@{#538507} [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/h264_decoder.cc [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/h264_decoder.h [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/h264_decoder_unittest.cc [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/v4l2/v4l2_slice_video_decode_accelerator.cc [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/v4l2/v4l2_slice_video_decode_accelerator.h [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vaapi/vaapi_h264_accelerator.cc [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vaapi/vaapi_h264_accelerator.h [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vaapi/vaapi_video_decode_accelerator.h [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vaapi/vaapi_vp8_accelerator.cc [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vaapi/vaapi_vp8_accelerator.h [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vaapi/vaapi_vp9_accelerator.cc [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vaapi/vaapi_vp9_accelerator.h [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vp8_decoder.cc [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vp8_decoder.h [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vp9_decoder.cc [modify] https://crrev.com/2d5ecad817b6ab0e2676c86c35e206889108963c/media/gpu/vp9_decoder.h
,
Feb 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c66b015961a082fceea968ba2ac7ff2f7cc4b9eb commit c66b015961a082fceea968ba2ac7ff2f7cc4b9eb Author: Miguel Casas <mcasas@chromium.org> Date: Tue Feb 27 17:11:34 2018 vaapi cleanup: move TRACE_COUNTER1(input buffers) to a better place This CL moves a TRACE_COUNTER1() counting input_buffers_.size() from its current position to next to |input_buffers_| actions (i.e. where it grows or shrinks). It also renames s/GetInputBuffer_Locked/GetCurrInputBuffer_Locked/ to better reflect what the method does. Cosmetic changes: - removes .get() on checks for |curr_input_buffer_| (bc unique_ptr<> has an operator boolean()). - Removes unnecessary curly brackets. - Removes superfluous comments. - Escapes some variable names in comments. TEST= simplechrome w/ crosvideos and v_d_a_unittests on soraka Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I5b3d51fd1509f8d0ce87487a44d6c7a77b4a253b Reviewed-on: https://chromium-review.googlesource.com/934403 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Cr-Commit-Position: refs/heads/master@{#539470} [modify] https://crrev.com/c66b015961a082fceea968ba2ac7ff2f7cc4b9eb/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/c66b015961a082fceea968ba2ac7ff2f7cc4b9eb/media/gpu/vaapi/vaapi_video_decode_accelerator.h
,
Feb 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/49e172d9c0bfa3c3708e4c7a65db070806985da0 commit 49e172d9c0bfa3c3708e4c7a65db070806985da0 Author: Miguel Casas <mcasas@chromium.org> Date: Tue Feb 27 22:35:46 2018 vaapi: remove unnecessary MEDIA_GPU_EXPORT clauses posciak@ brought to my attention that some classes under vaapi/ have MEDIA_GPU_EXPORT when they don't need to (they are not used beyond media_gpu component) and that's confusing and erroneous, so this CL removes them. TEST= no test needed since this is a build detail. Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I0397ff56d59380ebf55fc5595333b998faef9c4f Reviewed-on: https://chromium-review.googlesource.com/940104 Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Commit-Queue: Daniele Castagna <dcastagna@chromium.org> Cr-Commit-Position: refs/heads/master@{#539567} [modify] https://crrev.com/49e172d9c0bfa3c3708e4c7a65db070806985da0/media/gpu/vaapi/va_surface.h [modify] https://crrev.com/49e172d9c0bfa3c3708e4c7a65db070806985da0/media/gpu/vaapi/vaapi_decode_surface.h [modify] https://crrev.com/49e172d9c0bfa3c3708e4c7a65db070806985da0/media/gpu/vaapi/vaapi_h264_accelerator.h [modify] https://crrev.com/49e172d9c0bfa3c3708e4c7a65db070806985da0/media/gpu/vaapi/vaapi_vp8_accelerator.h [modify] https://crrev.com/49e172d9c0bfa3c3708e4c7a65db070806985da0/media/gpu/vaapi/vaapi_vp9_accelerator.h
,
Mar 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6683995b61a4915d9b91f9cb62ab348f95c6e5a2 commit 6683995b61a4915d9b91f9cb62ab348f95c6e5a2 Author: Miguel Casas <mcasas@chromium.org> Date: Wed Mar 14 00:05:31 2018 Vaapi: Remove ConditionVariables and use PostTask This CL removes the use of the ConditionVariables inside VaVDA and uses instead PostTask()ing. This removes blocking the worker thread, which allows for moving other tasks there, e.g. the Surface Blit (as done in the follow up WIP CL crrev.com/c/947341) For all that goodness, this CL: - Refactors the waiting logic that on ToT is spread between GetCurrInputBuffer_Locked() and WaitForSurfaces_Locked() and simplifies it in DecodeTask() itself. - Moves the |curr_input_buffer_| filling logic in GetCurrInputBuffer_Locked() into DecodeTask() proper. - Reduces the scoped of the |lock_| in DecodeTask() (note that |curr_input_buffer_| and |decode_| are only used on the decoder thread -- added a .h comment). Minor stuff: This CL also moves together the declaration of the related member vars |input_buffers_|, |curr_input_buffer_| and |available_va_surfaces_| TEST=simplechrome+crosvideo (including seeks) on soraka, and v_d_a_unittest with h264, vp8, vp9 (with dcheck_always_on=true). Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ib167115dd2a4ebaeaea3ba3b5a205779d659f479 Reviewed-on: https://chromium-review.googlesource.com/949283 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Cr-Commit-Position: refs/heads/master@{#542955} [modify] https://crrev.com/6683995b61a4915d9b91f9cb62ab348f95c6e5a2/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/6683995b61a4915d9b91f9cb62ab348f95c6e5a2/media/gpu/vaapi/vaapi_video_decode_accelerator.h
,
Mar 15 2018
,
Mar 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bc042703c1a038a4365e32e0480db53857a4bf6d commit bc042703c1a038a4365e32e0480db53857a4bf6d Author: Miguel Casas <mcasas@chromium.org> Date: Thu Mar 15 18:11:05 2018 vaapi: more UMAs VaVDA passes a UMA callback of sorts to VaapiWrapper to mark error conditions (Media.VAVDA.DecoderFailure) but there's no baseline for the amount of times it succeeds: this CL adds it as Media.VAVDA.VaapiWrapperCreationSuccess. It also adds a Media.VaapiWrapper.VADisplayStateInitializeSuccess to track the success/fail of initialising the VADisplayState, which we suspect to fail at times in the field. It also adds a "Vaapi" prefix to some traces so that they can be visually related in chrome:tracing. Bug: 717265 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ib841dca9626f2bed53a39b8bfd756d29c99b553e Reviewed-on: https://chromium-review.googlesource.com/962709 Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#543446} [modify] https://crrev.com/bc042703c1a038a4365e32e0480db53857a4bf6d/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/bc042703c1a038a4365e32e0480db53857a4bf6d/media/gpu/vaapi/vaapi_wrapper.cc [modify] https://crrev.com/bc042703c1a038a4365e32e0480db53857a4bf6d/tools/metrics/histograms/enums.xml [modify] https://crrev.com/bc042703c1a038a4365e32e0480db53857a4bf6d/tools/metrics/histograms/histograms.xml
,
Mar 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/618cb634a847958dec7b2bb3cbb94bbed6ebf031 commit 618cb634a847958dec7b2bb3cbb94bbed6ebf031 Author: Pawel Osciak <posciak@chromium.org> Date: Mon Mar 19 12:36:19 2018 Revert "Vaapi: Remove ConditionVariables and use PostTask" This reverts commit 6683995b61a4915d9b91f9cb62ab348f95c6e5a2. Reason for revert: crbug.com/823235 Original change's description: > Vaapi: Remove ConditionVariables and use PostTask > > This CL removes the use of the ConditionVariables inside VaVDA and > uses instead PostTask()ing. This removes blocking the worker > thread, which allows for moving other tasks there, e.g. the > Surface Blit (as done in the follow up WIP CL crrev.com/c/947341) > > For all that goodness, this CL: > > - Refactors the waiting logic that on ToT is spread between > GetCurrInputBuffer_Locked() and WaitForSurfaces_Locked() and > simplifies it in DecodeTask() itself. > > - Moves the |curr_input_buffer_| filling logic in > GetCurrInputBuffer_Locked() into DecodeTask() proper. > > - Reduces the scoped of the |lock_| in DecodeTask() (note that > |curr_input_buffer_| and |decode_| are only used on the decoder > thread -- added a .h comment). > > Minor stuff: This CL also moves together the declaration of the > related member vars |input_buffers_|, |curr_input_buffer_| and > |available_va_surfaces_| > > TEST=simplechrome+crosvideo (including seeks) on soraka, and > v_d_a_unittest with h264, vp8, vp9 (with dcheck_always_on=true). > > Bug: 717265 > Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel > Change-Id: Ib167115dd2a4ebaeaea3ba3b5a205779d659f479 > Reviewed-on: https://chromium-review.googlesource.com/949283 > Commit-Queue: Miguel Casas <mcasas@chromium.org> > Reviewed-by: Daniele Castagna <dcastagna@chromium.org> > Cr-Commit-Position: refs/heads/master@{#542955} TBR=posciak@chromium.org,kcwu@chromium.org,mcasas@chromium.org,dcastagna@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 717265 Cq-Include-Trybots: luci.chromium.try:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I62361a33067140eb42112ccf7613136137bc30e2 Reviewed-on: https://chromium-review.googlesource.com/967967 Commit-Queue: Pawel Osciak <posciak@chromium.org> Reviewed-by: Pawel Osciak <posciak@chromium.org> Cr-Commit-Position: refs/heads/master@{#544014} [modify] https://crrev.com/618cb634a847958dec7b2bb3cbb94bbed6ebf031/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/618cb634a847958dec7b2bb3cbb94bbed6ebf031/media/gpu/vaapi/vaapi_video_decode_accelerator.h
,
Mar 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5eec367971c0b4d21328f4798ed304c240d89e72 commit 5eec367971c0b4d21328f4798ed304c240d89e72 Author: Miguel Casas <mcasas@chromium.org> Date: Thu Mar 29 14:37:21 2018 RELAND: Vaapi: Remove ConditionVariables and use PostTask This CL relands the attached CL that got reverted due to scaling issues, i.e. when the decode bitstream changed resolution, there was the chance that before marking |awaiting_va_surfaces_recycle_|, we could get an "old" resolution frame, that would confuse |decoder_|. The solution is to move |awaiting_va_surfaces_recycle_| marking to DecodeTask(), and hold off any more Decode() operations until the "old" resolution frames are processed. This in turn makes InitiateSurfaceSetChange() superfluous, so it's moved into DecodeTask(). The rest is comment update :-) ** Changes can be found here: crrev.com/c/973684/3..9 TEST=http://crosvideo.appspot.com/?codec=vp9&cycle=true&loop=true&mute=true and letting it roll through many many many resolution changes. Original CL description ------------------------------------------------ This CL removes the use of the ConditionVariables inside VaVDA and uses instead PostTask()ing. This removes blocking the worker thread, which allows for moving other tasks there, e.g. the Surface Blit (as done in the follow up WIP CL crrev.com/c/947341) For all that goodness, this CL: - Refactors the waiting logic that on ToT is spread between GetCurrInputBuffer_Locked() and WaitForSurfaces_Locked() and simplifies it in DecodeTask() itself. - Moves the |curr_input_buffer_| filling logic in GetCurrInputBuffer_Locked() into DecodeTask() proper. - Reduces the scoped of the |lock_| in DecodeTask() (note that |curr_input_buffer_| and |decode_| are only used on the decoder thread -- added a .h comment). Minor stuff: This CL also moves together the declaration of the related member vars |input_buffers_|, |curr_input_buffer_| and |available_va_surfaces_| TEST=simplechrome+crosvideo (including seeks) on soraka, and v_d_a_unittest with h264, vp8, vp9 (with dcheck_always_on=true). Bug: 717265 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I86d179ac4d48c63b36787f5c7bdcb0da3839ed69 Reviewed-on: https://chromium-review.googlesource.com/949283 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#542955} Reviewed-on: https://chromium-review.googlesource.com/973684 Cr-Commit-Position: refs/heads/master@{#546827} [modify] https://crrev.com/5eec367971c0b4d21328f4798ed304c240d89e72/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/5eec367971c0b4d21328f4798ed304c240d89e72/media/gpu/vaapi/vaapi_video_decode_accelerator.h
,
Apr 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/83bd765601072b55ab31b8b23665a31578deca19 commit 83bd765601072b55ab31b8b23665a31578deca19 Author: Miguel Casas <mcasas@chromium.org> Date: Thu Apr 12 12:50:53 2018 vaapi: Don't use = {} to clear base::queues This CL removes the use of = {} to clear base::queues, because, for classes, zero initialization simply clears the (non-static) data members, which might leave the objects in an incorrect meta state because ctors are not called: this should not be an issue for std::queue, but it is for base::queue, which is implemented as a base::circular_deque. Nothing is crashing now, but a sibling unlanded sister CL (crrev.com/c/947341) had some issues with a similar idiom, so I'm removing it just in case. [1] http://en.cppreference.com/w/cpp/language/zero_initialization Bug: 717265 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I96af3e6475c0596f3316219bd51732e085f5e098 Reviewed-on: https://chromium-review.googlesource.com/1005917 Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#550162} [modify] https://crrev.com/83bd765601072b55ab31b8b23665a31578deca19/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
,
Apr 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/051ec184287440813ad2ae4f515f6d99a16e764d commit 051ec184287440813ad2ae4f515f6d99a16e764d Author: Miguel Casas <mcasas@chromium.org> Date: Mon Apr 16 16:39:01 2018 VaVDA: Move BlitSurface to |decoder_thread_task_runner_| This CL moves the call to VaapiPicture::DownloadFromSurface, which in turn calls VaapiWrapper::BlitSurface(), to be executed in the decoder thread of VaVDA. This considerable offloads the Gpu main thread for more urgent tasks, and anyway the said decoder thread is idle most of the time (seen in chrome:trace), so utilizing it more doesn't impact the overall performance but increases responsiveness. This Chrome:tracing details shows the change (soraka vp9 720p@30) before: https://i.imgur.com/CkqXqTp.png (https://imgur.com/a/pYVpf) after: https://i.imgur.com/o0c4ksW.png (https://imgur.com/a/wblx0) (the actual duration is inconsequential since it depends on the actual frame). nit: TRACE_EVENT1 is moved to the Vaapi*Picture because it results in easier syntax around the PostTaskAndReplyWithResult(). TEST= simplechrome+crosvideo and v_d_a_unittest h264/vp8/vp9 on soraka. No perf regression observed in the produced numbers. Bug: 717265 Cq-Include-Trybots: luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Icc4af2ef96141cebf95ac8fbb89c6e386e194a30 Reviewed-on: https://chromium-review.googlesource.com/947341 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Cr-Commit-Position: refs/heads/master@{#551006} [modify] https://crrev.com/051ec184287440813ad2ae4f515f6d99a16e764d/media/gpu/vaapi/vaapi_picture_native_pixmap.cc [modify] https://crrev.com/051ec184287440813ad2ae4f515f6d99a16e764d/media/gpu/vaapi/vaapi_picture_tfp.cc [modify] https://crrev.com/051ec184287440813ad2ae4f515f6d99a16e764d/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/051ec184287440813ad2ae4f515f6d99a16e764d/media/gpu/vaapi/vaapi_video_decode_accelerator.h [modify] https://crrev.com/051ec184287440813ad2ae4f515f6d99a16e764d/media/gpu/video_decode_accelerator_unittest.cc
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/051ec184287440813ad2ae4f515f6d99a16e764d commit 051ec184287440813ad2ae4f515f6d99a16e764d Author: Miguel Casas <mcasas@chromium.org> Date: Mon Apr 16 16:39:01 2018 VaVDA: Move BlitSurface to |decoder_thread_task_runner_| This CL moves the call to VaapiPicture::DownloadFromSurface, which in turn calls VaapiWrapper::BlitSurface(), to be executed in the decoder thread of VaVDA. This considerable offloads the Gpu main thread for more urgent tasks, and anyway the said decoder thread is idle most of the time (seen in chrome:trace), so utilizing it more doesn't impact the overall performance but increases responsiveness. This Chrome:tracing details shows the change (soraka vp9 720p@30) before: https://i.imgur.com/CkqXqTp.png (https://imgur.com/a/pYVpf) after: https://i.imgur.com/o0c4ksW.png (https://imgur.com/a/wblx0) (the actual duration is inconsequential since it depends on the actual frame). nit: TRACE_EVENT1 is moved to the Vaapi*Picture because it results in easier syntax around the PostTaskAndReplyWithResult(). TEST= simplechrome+crosvideo and v_d_a_unittest h264/vp8/vp9 on soraka. No perf regression observed in the produced numbers. Bug: 717265 Cq-Include-Trybots: luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Icc4af2ef96141cebf95ac8fbb89c6e386e194a30 Reviewed-on: https://chromium-review.googlesource.com/947341 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Cr-Commit-Position: refs/heads/master@{#551006} [modify] https://crrev.com/051ec184287440813ad2ae4f515f6d99a16e764d/media/gpu/vaapi/vaapi_picture_native_pixmap.cc [modify] https://crrev.com/051ec184287440813ad2ae4f515f6d99a16e764d/media/gpu/vaapi/vaapi_picture_tfp.cc [modify] https://crrev.com/051ec184287440813ad2ae4f515f6d99a16e764d/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/051ec184287440813ad2ae4f515f6d99a16e764d/media/gpu/vaapi/vaapi_video_decode_accelerator.h [modify] https://crrev.com/051ec184287440813ad2ae4f515f6d99a16e764d/media/gpu/video_decode_accelerator_unittest.cc
,
Apr 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e54827d909805f80bc26fe171dcfa3644c2085c9 commit e54827d909805f80bc26fe171dcfa3644c2085c9 Author: Miguel Casas <mcasas@chromium.org> Date: Thu Apr 19 19:27:55 2018 Revert "VaVDA: Move BlitSurface to |decoder_thread_task_runner_|" This reverts commit 051ec184287440813ad2ae4f515f6d99a16e764d. Reason for revert: Needed to revert other CLs that caused a regression in video_VideoSeek: https://crbug.com/834146 Bug: 834146 Original change's description: > VaVDA: Move BlitSurface to |decoder_thread_task_runner_| > > This CL moves the call to VaapiPicture::DownloadFromSurface, > which in turn calls VaapiWrapper::BlitSurface(), to be executed > in the decoder thread of VaVDA. This considerable offloads the > Gpu main thread for more urgent tasks, and anyway the said > decoder thread is idle most of the time (seen in chrome:trace), > so utilizing it more doesn't impact the overall performance but > increases responsiveness. > > This Chrome:tracing details shows the change (soraka vp9 720p@30) > before: https://i.imgur.com/CkqXqTp.png (https://imgur.com/a/pYVpf) > after: https://i.imgur.com/o0c4ksW.png (https://imgur.com/a/wblx0) > (the actual duration is inconsequential since it depends on the > actual frame). > > nit: TRACE_EVENT1 is moved to the Vaapi*Picture because it results > in easier syntax around the PostTaskAndReplyWithResult(). > > TEST= simplechrome+crosvideo and v_d_a_unittest h264/vp8/vp9 > on soraka. No perf regression observed in the produced numbers. > > Bug: 717265 > Cq-Include-Trybots: luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel > Change-Id: Icc4af2ef96141cebf95ac8fbb89c6e386e194a30 > Reviewed-on: https://chromium-review.googlesource.com/947341 > Commit-Queue: Miguel Casas <mcasas@chromium.org> > Reviewed-by: Daniele Castagna <dcastagna@chromium.org> > Cr-Commit-Position: refs/heads/master@{#551006} TBR=mcasas@chromium.org,dcastagna@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 717265 Change-Id: I86b367bc2b5d596b4f755440c494656555fa796f Cq-Include-Trybots: luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1020000 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#552114} [modify] https://crrev.com/e54827d909805f80bc26fe171dcfa3644c2085c9/media/gpu/vaapi/vaapi_picture_native_pixmap.cc [modify] https://crrev.com/e54827d909805f80bc26fe171dcfa3644c2085c9/media/gpu/vaapi/vaapi_picture_tfp.cc [modify] https://crrev.com/e54827d909805f80bc26fe171dcfa3644c2085c9/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/e54827d909805f80bc26fe171dcfa3644c2085c9/media/gpu/vaapi/vaapi_video_decode_accelerator.h [modify] https://crrev.com/e54827d909805f80bc26fe171dcfa3644c2085c9/media/gpu/video_decode_accelerator_unittest.cc
,
Apr 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e1aa3d31b61440058b3f157622d963ec05a4e97 commit 4e1aa3d31b61440058b3f157622d963ec05a4e97 Author: Miguel Casas <mcasas@chromium.org> Date: Thu Apr 19 19:46:10 2018 Revert "vaapi: Don't use = {} to clear base::queues" This reverts commit 83bd765601072b55ab31b8b23665a31578deca19. Reason for revert: Needed to revert other CLs that caused a regression in video_VideoSeek: https://crbug.com/834146 Bug: 834146 Original change's description: > vaapi: Don't use = {} to clear base::queues > > This CL removes the use of = {} to clear base::queues, because, for classes, > zero initialization simply clears the (non-static) data members, which might > leave the objects in an incorrect meta state because ctors are not called: > this should not be an issue for std::queue, but it is for base::queue, which > is implemented as a base::circular_deque. > > Nothing is crashing now, but a sibling unlanded sister CL (crrev.com/c/947341) > had some issues with a similar idiom, so I'm removing it just in case. > > [1] http://en.cppreference.com/w/cpp/language/zero_initialization > > Bug: 717265 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel > Change-Id: I96af3e6475c0596f3316219bd51732e085f5e098 > Reviewed-on: https://chromium-review.googlesource.com/1005917 > Reviewed-by: Daniele Castagna <dcastagna@chromium.org> > Commit-Queue: Miguel Casas <mcasas@chromium.org> > Cr-Commit-Position: refs/heads/master@{#550162} TBR=mcasas@chromium.org,dcastagna@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 717265 Change-Id: I06bc48deafedb9badd0937f9a2efd37c318edf6d Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1020001 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#552124} [modify] https://crrev.com/4e1aa3d31b61440058b3f157622d963ec05a4e97/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
,
Apr 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a1b30a7a9f2fa342058b128c55c37c27a878f416 commit a1b30a7a9f2fa342058b128c55c37c27a878f416 Author: Miguel Casas <mcasas@chromium.org> Date: Thu Apr 19 22:45:36 2018 Revert "RELAND: Vaapi: Remove ConditionVariables and use PostTask" This reverts commit 5eec367971c0b4d21328f4798ed304c240d89e72. Reason for revert: Caused regression in video_VideoSeek: https://crbug.com/834146 Bug: 834146 Original change's description: > RELAND: Vaapi: Remove ConditionVariables and use PostTask > > This CL relands the attached CL that got reverted due to scaling > issues, i.e. when the decode bitstream changed resolution, there > was the chance that before marking |awaiting_va_surfaces_recycle_|, > we could get an "old" resolution frame, that would confuse |decoder_|. > > The solution is to move |awaiting_va_surfaces_recycle_| marking to > DecodeTask(), and hold off any more Decode() operations until the > "old" resolution frames are processed. This in turn makes > InitiateSurfaceSetChange() superfluous, so it's moved into DecodeTask(). > The rest is comment update :-) > ** Changes can be found here: crrev.com/c/973684/3..9 > > TEST=http://crosvideo.appspot.com/?codec=vp9&cycle=true&loop=true&mute=true > and letting it roll through many many many resolution changes. > > Original CL description ------------------------------------------------ > > This CL removes the use of the ConditionVariables inside VaVDA and > uses instead PostTask()ing. This removes blocking the worker > thread, which allows for moving other tasks there, e.g. the > Surface Blit (as done in the follow up WIP CL crrev.com/c/947341) > > For all that goodness, this CL: > > - Refactors the waiting logic that on ToT is spread between > GetCurrInputBuffer_Locked() and WaitForSurfaces_Locked() and > simplifies it in DecodeTask() itself. > > - Moves the |curr_input_buffer_| filling logic in > GetCurrInputBuffer_Locked() into DecodeTask() proper. > > - Reduces the scoped of the |lock_| in DecodeTask() (note that > |curr_input_buffer_| and |decode_| are only used on the decoder > thread -- added a .h comment). > > Minor stuff: This CL also moves together the declaration of the > related member vars |input_buffers_|, |curr_input_buffer_| and > |available_va_surfaces_| > > TEST=simplechrome+crosvideo (including seeks) on soraka, and > v_d_a_unittest with h264, vp8, vp9 (with dcheck_always_on=true). > > Bug: 717265 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel > Change-Id: I86d179ac4d48c63b36787f5c7bdcb0da3839ed69 > Reviewed-on: https://chromium-review.googlesource.com/949283 > Commit-Queue: Miguel Casas <mcasas@chromium.org> > Reviewed-by: Daniele Castagna <dcastagna@chromium.org> > Cr-Original-Commit-Position: refs/heads/master@{#542955} > Reviewed-on: https://chromium-review.googlesource.com/973684 > Cr-Commit-Position: refs/heads/master@{#546827} TBR=posciak@chromium.org,mcasas@chromium.org,dcastagna@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 717265 Change-Id: Ic31471daf757fd08ff2f420afc5be7e973a88c01 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1020040 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#552190} [modify] https://crrev.com/a1b30a7a9f2fa342058b128c55c37c27a878f416/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/a1b30a7a9f2fa342058b128c55c37c27a878f416/media/gpu/vaapi/vaapi_video_decode_accelerator.h
,
Apr 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2ed1cbcb8bcfc393bb947740fc5e53a2023d2ad6 commit 2ed1cbcb8bcfc393bb947740fc5e53a2023d2ad6 Author: Miguel Casas <mcasas@chromium.org> Date: Fri Apr 20 22:04:22 2018 Revert "vaapi: Don't use = {} to clear base::queues" This reverts commit 83bd765601072b55ab31b8b23665a31578deca19. Reason for revert: Needed to revert other CLs that caused a regression in video_VideoSeek: https://crbug.com/834146 Bug: 834146 Original change's description: > vaapi: Don't use = {} to clear base::queues > > This CL removes the use of = {} to clear base::queues, because, for classes, > zero initialization simply clears the (non-static) data members, which might > leave the objects in an incorrect meta state because ctors are not called: > this should not be an issue for std::queue, but it is for base::queue, which > is implemented as a base::circular_deque. > > Nothing is crashing now, but a sibling unlanded sister CL (crrev.com/c/947341) > had some issues with a similar idiom, so I'm removing it just in case. > > [1] http://en.cppreference.com/w/cpp/language/zero_initialization > > Bug: 717265 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel > Change-Id: I96af3e6475c0596f3316219bd51732e085f5e098 > Reviewed-on: https://chromium-review.googlesource.com/1005917 > Reviewed-by: Daniele Castagna <dcastagna@chromium.org> > Commit-Queue: Miguel Casas <mcasas@chromium.org> > Cr-Commit-Position: refs/heads/master@{#550162} TBR=mcasas@chromium.org,dcastagna@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 717265 Change-Id: I06bc48deafedb9badd0937f9a2efd37c318edf6d Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1020001 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Miguel Casas <mcasas@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#552124}(cherry picked from commit 4e1aa3d31b61440058b3f157622d963ec05a4e97) Reviewed-on: https://chromium-review.googlesource.com/1022890 Cr-Commit-Position: refs/branch-heads/3396@{#177} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/2ed1cbcb8bcfc393bb947740fc5e53a2023d2ad6/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
,
Apr 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2296de366e227d2bfa466159061ebbef6d309394 commit 2296de366e227d2bfa466159061ebbef6d309394 Author: Miguel Casas <mcasas@chromium.org> Date: Fri Apr 20 22:08:30 2018 Revert "RELAND: Vaapi: Remove ConditionVariables and use PostTask" This reverts commit 5eec367971c0b4d21328f4798ed304c240d89e72. Reason for revert: Caused regression in video_VideoSeek: https://crbug.com/834146 Bug: 834146 Original change's description: > RELAND: Vaapi: Remove ConditionVariables and use PostTask > > This CL relands the attached CL that got reverted due to scaling > issues, i.e. when the decode bitstream changed resolution, there > was the chance that before marking |awaiting_va_surfaces_recycle_|, > we could get an "old" resolution frame, that would confuse |decoder_|. > > The solution is to move |awaiting_va_surfaces_recycle_| marking to > DecodeTask(), and hold off any more Decode() operations until the > "old" resolution frames are processed. This in turn makes > InitiateSurfaceSetChange() superfluous, so it's moved into DecodeTask(). > The rest is comment update :-) > ** Changes can be found here: crrev.com/c/973684/3..9 > > TEST=http://crosvideo.appspot.com/?codec=vp9&cycle=true&loop=true&mute=true > and letting it roll through many many many resolution changes. > > Original CL description ------------------------------------------------ > > This CL removes the use of the ConditionVariables inside VaVDA and > uses instead PostTask()ing. This removes blocking the worker > thread, which allows for moving other tasks there, e.g. the > Surface Blit (as done in the follow up WIP CL crrev.com/c/947341) > > For all that goodness, this CL: > > - Refactors the waiting logic that on ToT is spread between > GetCurrInputBuffer_Locked() and WaitForSurfaces_Locked() and > simplifies it in DecodeTask() itself. > > - Moves the |curr_input_buffer_| filling logic in > GetCurrInputBuffer_Locked() into DecodeTask() proper. > > - Reduces the scoped of the |lock_| in DecodeTask() (note that > |curr_input_buffer_| and |decode_| are only used on the decoder > thread -- added a .h comment). > > Minor stuff: This CL also moves together the declaration of the > related member vars |input_buffers_|, |curr_input_buffer_| and > |available_va_surfaces_| > > TEST=simplechrome+crosvideo (including seeks) on soraka, and > v_d_a_unittest with h264, vp8, vp9 (with dcheck_always_on=true). > > Bug: 717265 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel > Change-Id: I86d179ac4d48c63b36787f5c7bdcb0da3839ed69 > Reviewed-on: https://chromium-review.googlesource.com/949283 > Commit-Queue: Miguel Casas <mcasas@chromium.org> > Reviewed-by: Daniele Castagna <dcastagna@chromium.org> > Cr-Original-Commit-Position: refs/heads/master@{#542955} > Reviewed-on: https://chromium-review.googlesource.com/973684 > Cr-Commit-Position: refs/heads/master@{#546827} TBR=posciak@chromium.org,mcasas@chromium.org,dcastagna@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 717265 Change-Id: Ic31471daf757fd08ff2f420afc5be7e973a88c01 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1020040 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Miguel Casas <mcasas@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#552190}(cherry picked from commit a1b30a7a9f2fa342058b128c55c37c27a878f416) Reviewed-on: https://chromium-review.googlesource.com/1022912 Cr-Commit-Position: refs/branch-heads/3396@{#182} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/2296de366e227d2bfa466159061ebbef6d309394/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/2296de366e227d2bfa466159061ebbef6d309394/media/gpu/vaapi/vaapi_video_decode_accelerator.h
,
May 1 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6cc4e64c3ba33b4dc34eae1cca5e97419799271e commit 6cc4e64c3ba33b4dc34eae1cca5e97419799271e Author: Miguel Casas <mcasas@chromium.org> Date: Tue May 01 19:36:21 2018 vaapi: Add test case for input resolution change This CL adds a test case for the case of resolution change (which bit me in crbug.com/834146 ). In the process, a few common VaVDA<->test harness subsequences are factored out into methods: - QueueInputBufferSequence - AssignPictureBuffersSequence - DecodeOneFrameFast which allows to simplify other test cases. Finally, the test case SupportedPlatforms() is moved around untouched. Bug: 717265 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Ic19d89c1355eb74d12658f5fee04eaf17ee39c6e Reviewed-on: https://chromium-review.googlesource.com/1020303 Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#555148} [modify] https://crrev.com/6cc4e64c3ba33b4dc34eae1cca5e97419799271e/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
,
May 1 2018
,
May 1 2018
,
Jun 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6873824cc1c2edee42ee3d0dfac8f0ba77810320 commit 6873824cc1c2edee42ee3d0dfac8f0ba77810320 Author: Miguel Casas <mcasas@chromium.org> Date: Mon Jun 04 23:31:47 2018 VaapiPicture*: add missing DCHECK()s This CL adds 3 missing DCHECK()s that I forgot after [1] [1] https://chromium-review.googlesource.com/c/chromium/src/+/1079834/13/media/gpu/vaapi/vaapi_picture_factory.cc#50 TBR=dcastagna@chromium.org Bug: 717265 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Ibfc9797bda93dd7660cc64317ab21794dbda6e10 Reviewed-on: https://chromium-review.googlesource.com/1086131 Reviewed-by: Miguel Casas <mcasas@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#564289} [modify] https://crrev.com/6873824cc1c2edee42ee3d0dfac8f0ba77810320/media/gpu/vaapi/vaapi_picture_native_pixmap_egl.cc [modify] https://crrev.com/6873824cc1c2edee42ee3d0dfac8f0ba77810320/media/gpu/vaapi/vaapi_picture_native_pixmap_ozone.cc [modify] https://crrev.com/6873824cc1c2edee42ee3d0dfac8f0ba77810320/media/gpu/vaapi/vaapi_picture_tfp.cc
,
Jun 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bdbda14327485848579f9b0d949457c0c62cd05a commit bdbda14327485848579f9b0d949457c0c62cd05a Author: Hirokazu Honda <hiroh@chromium.org> Date: Mon Jun 11 03:34:38 2018 Revert "VaapiPicture*: add missing DCHECK()s" This reverts commit 6873824cc1c2edee42ee3d0dfac8f0ba77810320. Reason for revert: crrev.com/c/1079834 breaks ARC++ video playback. This CL has dependency of the CL. Original change's description: > VaapiPicture*: add missing DCHECK()s > > This CL adds 3 missing DCHECK()s that I forgot after [1] > > [1] https://chromium-review.googlesource.com/c/chromium/src/+/1079834/13/media/gpu/vaapi/vaapi_picture_factory.cc#50 > > TBR=dcastagna@chromium.org > > Bug: 717265 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel > Change-Id: Ibfc9797bda93dd7660cc64317ab21794dbda6e10 > Reviewed-on: https://chromium-review.googlesource.com/1086131 > Reviewed-by: Miguel Casas <mcasas@chromium.org> > Commit-Queue: Miguel Casas <mcasas@chromium.org> > Cr-Commit-Position: refs/heads/master@{#564289} TBR=mcasas@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 717265, b:109906289 Change-Id: Ibc20bee25cdfa127d0b646bc1a03a7e53a6129e4 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1094794 Commit-Queue: Hirokazu Honda <hiroh@chromium.org> Reviewed-by: Pawel Osciak <posciak@chromium.org> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> Cr-Commit-Position: refs/heads/master@{#565921} [modify] https://crrev.com/bdbda14327485848579f9b0d949457c0c62cd05a/media/gpu/vaapi/vaapi_picture_native_pixmap_egl.cc [modify] https://crrev.com/bdbda14327485848579f9b0d949457c0c62cd05a/media/gpu/vaapi/vaapi_picture_native_pixmap_ozone.cc [modify] https://crrev.com/bdbda14327485848579f9b0d949457c0c62cd05a/media/gpu/vaapi/vaapi_picture_tfp.cc
,
Jun 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c1ee562516e98cb7066bca4614f94ea7beec06aa commit c1ee562516e98cb7066bca4614f94ea7beec06aa Author: Miguel Casas <mcasas@chromium.org> Date: Wed Jun 20 16:53:02 2018 vaapi: decode on client NativePixmaps This CL teaches VaVDA to use client VASurfaceIDs to decode onto, saving a buffer copy and removing a costly blit (DownloadFromSurface) on the GPU Main thread. Three groups of changes: 1. In AssignPictureBuffers(): if |vaapi_picture_factory_| Create()s a VaapiPicture with a VASurfaceID, we use those to decode onto and set a new flag: |decode_using_client_picture_buffers_| 2. When the decoder calls CreateVASurface(), instead of giving it the first |available_va_surfaces_|, we need to figure out the first VASurfaceID in this |available_va_surfaces_| such that the corresponding VaapiPicture in |pictures_| is available (i.e. in |available_picture_buffers_|). Reason: libva holds on to some VASurfaceIDs, there's no simple one-to-one correspondence like on ToT. 3. When we're ready to OutputPicture() to |client_|, instead of using the first |available_picture_buffers_|, we find the corresponding one for the passed |va_surface_id| (we could search all over |pictures_| but there's always less |available_picture_buffers_|. Some other notable developments: - s/output_buffers_/available_picture_buffers_/ - OutputPicture() loses a parameter. - |decode_using_client_picture_buffers_| is still false for e.g. VP9 Profile 2 or the ImportBufferForPicture() path. - Pictures is made a base::small_map. (Originally from crrev.com/c/988512). Bug: 822346, 717265 Test: v_d_a_unittest vp8/vp9/h264 passing on eve, running crosvideo changing resolutions for a long time. No hickups, no dropped frames. Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I3d5a4d3c83002c8d0977702c9b97ffec52b1db23 Reviewed-on: https://chromium-review.googlesource.com/1021675 Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#568880} [modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_picture.cc [modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_picture.h [modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_picture_native_pixmap.cc [modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_picture_native_pixmap.h [modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_video_decode_accelerator.h [modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
,
Jun 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9921f96141b3172c6a38dd1f42d4acfb0507ae96 commit 9921f96141b3172c6a38dd1f42d4acfb0507ae96 Author: Miguel Casas <mcasas@chromium.org> Date: Wed Jun 27 13:56:44 2018 Revert "vaapi: decode on client NativePixmaps" This reverts commit c1ee562516e98cb7066bca4614f94ea7beec06aa. Reason for revert: The necessary predecessor crrev.com/c/1104394 has been reverted (for other reasons, see crbug.com/822346#c33) Original change's description: > vaapi: decode on client NativePixmaps > > This CL teaches VaVDA to use client VASurfaceIDs to decode onto, saving > a buffer copy and removing a costly blit (DownloadFromSurface) on the > GPU Main thread. Three groups of changes: > > 1. In AssignPictureBuffers(): if |vaapi_picture_factory_| Create()s a > VaapiPicture with a VASurfaceID, we use those to decode onto and set a new > flag: |decode_using_client_picture_buffers_| > > 2. When the decoder calls CreateVASurface(), instead of giving it the first > |available_va_surfaces_|, we need to figure out the first VASurfaceID in > this |available_va_surfaces_| such that the corresponding VaapiPicture in > |pictures_| is available (i.e. in |available_picture_buffers_|). > Reason: libva holds on to some VASurfaceIDs, there's no simple one-to-one > correspondence like on ToT. > > 3. When we're ready to OutputPicture() to |client_|, instead of using the first > |available_picture_buffers_|, we find the corresponding one for the > passed |va_surface_id| (we could search all over |pictures_| but there's > always less |available_picture_buffers_|. > > Some other notable developments: > > - s/output_buffers_/available_picture_buffers_/ > - OutputPicture() loses a parameter. > - |decode_using_client_picture_buffers_| is still false for e.g. VP9 Profile 2 > or the ImportBufferForPicture() path. > - Pictures is made a base::small_map. (Originally from crrev.com/c/988512). > > Bug: 822346, 717265 > Test: v_d_a_unittest vp8/vp9/h264 passing on eve, running crosvideo changing > resolutions for a long time. No hickups, no dropped frames. > > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel > Change-Id: I3d5a4d3c83002c8d0977702c9b97ffec52b1db23 > Reviewed-on: https://chromium-review.googlesource.com/1021675 > Reviewed-by: Daniele Castagna <dcastagna@chromium.org> > Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org> > Commit-Queue: Miguel Casas <mcasas@chromium.org> > Cr-Commit-Position: refs/heads/master@{#568880} TBR=mcasas@chromium.org,dcastagna@chromium.org,hoegsberg@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 822346, 717265 Change-Id: I5e8cb9431f72a65f67782e59cb5ce52f7e1d4ce7 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1116938 Reviewed-by: Miguel Casas <mcasas@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#570750} [modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_picture.cc [modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_picture.h [modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_picture_native_pixmap.cc [modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_picture_native_pixmap.h [modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_video_decode_accelerator.h [modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
,
Jul 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae29b15515cf03a9b53086a141b8a070b03f6ae5 commit ae29b15515cf03a9b53086a141b8a070b03f6ae5 Author: Miguel Casas <mcasas@chromium.org> Date: Mon Jul 23 20:02:11 2018 RELAND: vaapi: decode on client NativePixmaps This CL relands crrev.com/c/1021675 that got reverted as predecessor to crrev.com/c/1104394 (but was otherwise innocent AFAIK). The said predecessor situation has been corrected by crrev.com/c/1117606 (that landed in ChromeOS 10840.0.0 [1]) [1] https://crosland.corp.google.com/log/10837.0.0..10840.0.0 TBR=dcastagna@chromium.org (same code as reviewed before) Original CL description ----------------------------------------------- vaapi: decode on client NativePixmaps This CL teaches VaVDA to use client VASurfaceIDs to decode onto, saving a buffer copy and removing a costly blit (DownloadFromSurface) on the GPU Main thread. Three groups of changes: 1. In AssignPictureBuffers(): if |vaapi_picture_factory_| Create()s a VaapiPicture with a VASurfaceID, we use those to decode onto and set a new flag: |decode_using_client_picture_buffers_| 2. When the decoder calls CreateVASurface(), instead of giving it the first |available_va_surfaces_|, we need to figure out the first VASurfaceID in this |available_va_surfaces_| such that the corresponding VaapiPicture in |pictures_| is available (i.e. in |available_picture_buffers_|). Reason: libva holds on to some VASurfaceIDs, there's no simple one-to-one correspondence like on ToT. 3. When we're ready to OutputPicture() to |client_|, instead of using the first |available_picture_buffers_|, we find the corresponding one for the passed |va_surface_id| (we could search all over |pictures_| but there's always less |available_picture_buffers_|. Some other notable developments: - s/output_buffers_/available_picture_buffers_/ - OutputPicture() loses a parameter. - |decode_using_client_picture_buffers_| is still false for e.g. VP9 Profile 2 or the ImportBufferForPicture() path. - Pictures is made a base::small_map. (Originally from crrev.com/c/988512). resolutions for a long time. No hickups, no dropped frames. Bug: 822346, 717265 Test: v_d_a_unittest vp8/vp9/h264 passing on eve, running crosvideo changing Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Ie1fb21a7a302476bceee3dd905278d53f13b1475 Reviewed-on: https://chromium-review.googlesource.com/1021675 Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1147143 Commit-Queue: Miguel Casas <mcasas@chromium.org> Reviewed-by: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#577226} [modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_picture.cc [modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_picture.h [modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_picture_native_pixmap.cc [modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_picture_native_pixmap.h [modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_video_decode_accelerator.h [modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
,
Aug 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/148b331759e9e346d6f214d63805832a1ee6232c commit 148b331759e9e346d6f214d63805832a1ee6232c Author: Pawel Osciak <posciak@chromium.org> Date: Fri Aug 03 16:26:12 2018 Revert "RELAND: vaapi: decode on client NativePixmaps" This reverts commit ae29b15515cf03a9b53086a141b8a070b03f6ae5. Reason for revert: crbug.com/868190. Original change's description: > RELAND: vaapi: decode on client NativePixmaps > > This CL relands crrev.com/c/1021675 that got reverted as predecessor > to crrev.com/c/1104394 (but was otherwise innocent AFAIK). The said > predecessor situation has been corrected by crrev.com/c/1117606 (that > landed in ChromeOS 10840.0.0 [1]) > > [1] https://crosland.corp.google.com/log/10837.0.0..10840.0.0 > > TBR=dcastagna@chromium.org (same code as reviewed before) > > Original CL description ----------------------------------------------- > vaapi: decode on client NativePixmaps > > This CL teaches VaVDA to use client VASurfaceIDs to decode onto, saving > a buffer copy and removing a costly blit (DownloadFromSurface) on the > GPU Main thread. Three groups of changes: > > 1. In AssignPictureBuffers(): if |vaapi_picture_factory_| Create()s a > VaapiPicture with a VASurfaceID, we use those to decode onto and set a new > flag: |decode_using_client_picture_buffers_| > > 2. When the decoder calls CreateVASurface(), instead of giving it the first > |available_va_surfaces_|, we need to figure out the first VASurfaceID in > this |available_va_surfaces_| such that the corresponding VaapiPicture in > |pictures_| is available (i.e. in |available_picture_buffers_|). > Reason: libva holds on to some VASurfaceIDs, there's no simple one-to-one > correspondence like on ToT. > > 3. When we're ready to OutputPicture() to |client_|, instead of using the first > |available_picture_buffers_|, we find the corresponding one for the > passed |va_surface_id| (we could search all over |pictures_| but there's > always less |available_picture_buffers_|. > > Some other notable developments: > > - s/output_buffers_/available_picture_buffers_/ > - OutputPicture() loses a parameter. > - |decode_using_client_picture_buffers_| is still false for e.g. VP9 Profile 2 > or the ImportBufferForPicture() path. > - Pictures is made a base::small_map. (Originally from crrev.com/c/988512). > > resolutions for a long time. No hickups, no dropped frames. > > Bug: 822346, 717265 > Test: v_d_a_unittest vp8/vp9/h264 passing on eve, running crosvideo changing > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel > Change-Id: Ie1fb21a7a302476bceee3dd905278d53f13b1475 > Reviewed-on: https://chromium-review.googlesource.com/1021675 > Reviewed-by: Daniele Castagna <dcastagna@chromium.org> > Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org> > Reviewed-on: https://chromium-review.googlesource.com/1147143 > Commit-Queue: Miguel Casas <mcasas@chromium.org> > Reviewed-by: Miguel Casas <mcasas@chromium.org> > Cr-Commit-Position: refs/heads/master@{#577226} TBR=mcasas@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 822346, 717265 Change-Id: I8b91c852d94061771fc0ff8caea6616c74f9454a Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1161621 Reviewed-by: Pawel Osciak <posciak@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#580567} [modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_picture.cc [modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_picture.h [modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_picture_native_pixmap.cc [modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_picture_native_pixmap.h [modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_video_decode_accelerator.h [modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
,
Aug 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b4f1b40236a3fa8f058b0ab3cf6e6a028f1ad7ec commit b4f1b40236a3fa8f058b0ab3cf6e6a028f1ad7ec Author: Miguel Casas <mcasas@chromium.org> Date: Thu Aug 16 01:05:42 2018 VaapiVDA: fold DecodeVASurface into callers VaapiVideoDecodeAccelerator::DecodeVASurface() is superfluous because the callers can do the call to |vaapi_wrapper_| themselves. Moreover, DecodeVaSurface() needs no logging because the underlying method ExecuteAndDestroyPendingBuffers() has already a lot of logs. Bug: 717265 Test: v_d_a vp8/vp9/h264 working as before on nautilus. simplechrome too. Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Ibc40329808e744f6ca7af98cb7de4e13a7af5016 Reviewed-on: https://chromium-review.googlesource.com/1176279 Reviewed-by: Emircan Uysaler <emircan@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#583477} [modify] https://crrev.com/b4f1b40236a3fa8f058b0ab3cf6e6a028f1ad7ec/media/gpu/vaapi/vaapi_h264_accelerator.cc [modify] https://crrev.com/b4f1b40236a3fa8f058b0ab3cf6e6a028f1ad7ec/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/b4f1b40236a3fa8f058b0ab3cf6e6a028f1ad7ec/media/gpu/vaapi/vaapi_video_decode_accelerator.h [modify] https://crrev.com/b4f1b40236a3fa8f058b0ab3cf6e6a028f1ad7ec/media/gpu/vaapi/vaapi_vp8_accelerator.cc [modify] https://crrev.com/b4f1b40236a3fa8f058b0ab3cf6e6a028f1ad7ec/media/gpu/vaapi/vaapi_vp9_accelerator.cc
,
Aug 16
,
Aug 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6727aea8c39de749494e520bc955416e0a2d0e77 commit 6727aea8c39de749494e520bc955416e0a2d0e77 Author: Miguel Casas <mcasas@chromium.org> Date: Sat Aug 18 00:23:23 2018 VaVDA: preparation for decode on client NativePixmaps This CL extracts a few cleanups/preparations of crrev.com/c/1171566 (that is a big CL and might get reverted a few times). - VaapiPicture::va_surface_id() allows for accessing the underlying VASurfaceID, if this is present (otherwise we return VA_INVALID_ID). - Pictures is made a base::small_map (originally crrev.com/c/988512), PictureById() is removed in favour of using base::ContainsKey() on 3 callsites. Bug: 822346, 717265 Test: v_d_a_unittest vp8/vp9/h264 passing on nautilus Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I18a0335462b321d25550d295cb628b4ddd3e2269 Reviewed-on: https://chromium-review.googlesource.com/1180295 Reviewed-by: Emircan Uysaler <emircan@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#584256} [modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_picture.cc [modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_picture.h [modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_picture_native_pixmap.cc [modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_picture_native_pixmap.h [modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_video_decode_accelerator.h
,
Aug 20
,
Aug 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/49dc6464052f4cc335ffb65895244836dec30673 commit 49dc6464052f4cc335ffb65895244836dec30673 Author: Miguel Casas <mcasas@chromium.org> Date: Mon Aug 20 20:31:41 2018 vaapi: decode on client NativePixmaps (reduced) crrev.com/c/1171566 got reverted due to a mishmash of performance improvements and regressions. Instead of relanding it verbatim, the innocent/innocuous parts were landed in crrev.com/c/1180295; this CL gets the tough parts, including restricting it to VP9 and Kabylakes. I also repeated the power consumption results (https://goo.gl/r88qPf), and in a nutshell I see improvements: w/ this CL: 1.88 0.63 0.41 0.52 (pkg - pp1 - gfx - dram) ToT : 2.72 0.76 1.03 0.57 Savings : 30.78% 17.51% 59.95% 7.60% TBR=dcastagna@, hoegsberg@ Original CL description ----------------------------------------------- vaapi: decode on client NativePixmaps This CL teaches VaVDA to use client VASurfaceIDs to decode onto, saving a buffer copy and removing a costly blit (DownloadFromSurface) on the GPU Main thread. Three groups of changes: 1. In AssignPictureBuffers(): if |vaapi_picture_factory_| Create()s a VaapiPicture with a VASurfaceID, we use those to decode onto and set a new flag: |decode_using_client_picture_buffers_| 2. When the decoder calls CreateVASurface(), instead of giving it the first |available_va_surfaces_|, we need to figure out the first VASurfaceID in this |available_va_surfaces_| such that the corresponding VaapiPicture in |pictures_| is available (i.e. in |available_picture_buffers_|). Reason: libva holds on to some VASurfaceIDs, there's no simple one-to-one correspondence like on ToT. 3. When we're ready to OutputPicture() to |client_|, instead of using the first |available_picture_buffers_|, we find the corresponding one for the passed |va_surface_id| (we could search all over |pictures_| but there's always less |available_picture_buffers_|. Some other notable developments: - s/output_buffers_/available_picture_buffers_/ - OutputPicture() loses a parameter. - |decode_using_client_picture_buffers_| is still false for e.g. VP9 Profile 2 or the ImportBufferForPicture() path. Bug: 822346, 717265 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Icf1a9a1fa71264a7367fa351e3b7d28fab346744 Reviewed-on: https://chromium-review.googlesource.com/1181464 Reviewed-by: Miguel Casas <mcasas@chromium.org> Commit-Queue: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#584553} [modify] https://crrev.com/49dc6464052f4cc335ffb65895244836dec30673/media/gpu/vaapi/vaapi_video_decode_accelerator.cc [modify] https://crrev.com/49dc6464052f4cc335ffb65895244836dec30673/media/gpu/vaapi/vaapi_video_decode_accelerator.h [modify] https://crrev.com/49dc6464052f4cc335ffb65895244836dec30673/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
,
Dec 14
[GPU Triage Council] This is in our P1 queue. Is this fixed by the above patches? Should it be moved to P2?
,
Dec 14
#67: not entirely, certain codecs + platforms are optimized to address the bug title and others are still WIP; but it's not P1 def'ly. |
||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by dcasta...@chromium.org
, May 2 2017