From f97ca58f1785e622f9a7ed6b046578a6e35f7c3a Mon Sep 17 00:00:00 2001 From: Guido Urdaneta Date: Tue, 18 Aug 2020 09:15:32 +0000 Subject: [PATCH] Revert "Reland "RELAND 2: media/gpu/vaapi_video_decoder: keep allocated VASurfaces alive"" This reverts commit 49422e897d817c3d8615bc4afe68c027c72e16c8. Reason for revert: Speculative revert. Suspect of causing some failures that report Gpu issues with Wayland. https://ci.chromium.org/p/chromium/builders/ci/linux-lacros-tester-rel/1109 Will reland if the revert does not fix the bot. Original change's description: > Reland "RELAND 2: media/gpu/vaapi_video_decoder: keep allocated VASurfaces alive" > > This is a reland of 02aec841cb5621fd2f8acb6a13e34e12e56b3a37 > > Speculative revert was incorrect > > Original change's description: > > RELAND 2: media/gpu/vaapi_video_decoder: keep allocated VASurfaces alive > > > > The relanded CL was reverted due to crashes when used from ARC++: > > Video Frames coming from this path are not GpuMemoryBuffers, so there's > > a DCHECK not verified, and then a crash. This CL addresses that > > situation, fixing `video_decode_accelerator_tests --use_vd_vda`: > > see crrev.com/c/2359734/1..3. > > > > Bug: b:163920993 > > > > Original RELAND description ------------------------------------------- > > > > The original CL was reverted due to crashes in betty initialization: > > betty is a VM which doesn't support VA, so the VideoDecoderPipeline > > constructed-destructed a VaapiVideoDecoder, hitting an unprotected > > nullptr |vaapi_wrapper_| in dtor. Fix in crrev.com/c/2339494/2..3. > > > > TBR=andrescj@chromium.org > > > > Original CL description ----------------------------------------------- > > > > Certain platforms and codecs suffer from horrible artifacts (Intel BYT, > > H264) or crashes (Intel BSW/BDW, VP9). This was traced to some kind of > > error in the tracking of the VASurfaces lifetime in the driver: every > > time we get a new resource from the pool to decode onto, this is > > imported into libva as a VASurface: this works fine almost everywhere > > but doesn't play well in these old platforms (see CreateSurface() body). > > > > This CL adds a map that keeps the ref-counted VASurfaces alive and > > indexed by the unique GpuMemoryBufferId until the VA Context is > > destroyed. In so doing, we're basically observing the "contract" of > > va.h vaDestroySurfaces() [1] "Surfaces can only be destroyed after all > > contexts using these surfaces have been destroyed". > > > > [1] https://github.com/intel/libva/blob/libva-2.0.0/va/va.h#L1134 > > > > Bug: b:142019786 b:143323596 > > Change-Id: I16e74eb2b483b892961eca27aed30112240aa8ba > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2339494 > > Reviewed-by: Miguel Casas > > Commit-Queue: Miguel Casas > > Cr-Original-Commit-Position: refs/heads/master@{#795026} > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2359734 > > Reviewed-by: Andres Calderon Jaramillo > > Commit-Queue: Andres Calderon Jaramillo > > Auto-Submit: Miguel Casas > > Cr-Commit-Position: refs/heads/master@{#798931} > > TBR=mcasas@chromium.org,andrescj@chromium.org > > Bug: b:163920993 > Bug: b:142019786 b:143323596 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Change-Id: I6124e960d93f6985dbf9a546bc9d34372815e002 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2359975 > Reviewed-by: Fergus Dall > Commit-Queue: Fergus Dall > Cr-Commit-Position: refs/heads/master@{#798968} TBR=mcasas@chromium.org,andrescj@chromium.org,sidereal@google.com Change-Id: I5a6c92e18abcfd4d16f67942fa882fdb6cbe4001 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: b:163920993 Bug: b:142019786 b:143323596 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362247 Reviewed-by: Guido Urdaneta Commit-Queue: Guido Urdaneta Cr-Commit-Position: refs/heads/master@{#799038} --- media/gpu/vaapi/vaapi_video_decoder.cc | 87 +++++++++----------------- media/gpu/vaapi/vaapi_video_decoder.h | 23 +++---- 2 files changed, 35 insertions(+), 75 deletions(-) diff --git a/media/gpu/vaapi/vaapi_video_decoder.cc b/media/gpu/vaapi/vaapi_video_decoder.cc index 3571c0a3d89069..06e29d466355c4 100644 --- a/media/gpu/vaapi/vaapi_video_decoder.cc +++ b/media/gpu/vaapi/vaapi_video_decoder.cc @@ -97,19 +97,6 @@ VaapiVideoDecoder::~VaapiVideoDecoder() { ClearDecodeTaskQueue(DecodeStatus::ABORTED); weak_this_factory_.InvalidateWeakPtrs(); - - // Destroy explicitly to DCHECK() that |vaapi_wrapper_| references are held - // inside the accelerator in |decoder_|, by the |allocated_va_surfaces_| and - // of course by this class. To clear |allocated_va_surfaces_| we have to first - // DestroyContext(). - decoder_ = nullptr; - if (vaapi_wrapper_) { - vaapi_wrapper_->DestroyContext(); - allocated_va_surfaces_.clear(); - - DCHECK(vaapi_wrapper_->HasOneRef()); - vaapi_wrapper_ = nullptr; - } } void VaapiVideoDecoder::Initialize(const VideoDecoderConfig& config, @@ -137,12 +124,6 @@ void VaapiVideoDecoder::Initialize(const VideoDecoderConfig& config, DVLOGF(3) << "Reinitializing decoder"; decoder_ = nullptr; - DCHECK(vaapi_wrapper_); - // To clear |allocated_va_surfaces_| we have to first DestroyContext(). - vaapi_wrapper_->DestroyContext(); - allocated_va_surfaces_.clear(); - - DCHECK(vaapi_wrapper_->HasOneRef()); vaapi_wrapper_ = nullptr; decoder_delegate_ = nullptr; SetState(State::kUninitialized); @@ -319,36 +300,22 @@ scoped_refptr VaapiVideoDecoder::CreateSurface() { return nullptr; } - // |frame|s coming from ARC++ are not GpuMemoryBuffer-backed, but they have - // DmaBufs whose fd numbers are consistent along the lifetime of the VA - // surfaces they back. - DCHECK(frame->GetGpuMemoryBuffer() || frame->HasDmaBufs()); - const gfx::GpuMemoryBufferId frame_id = - frame->GetGpuMemoryBuffer() - ? frame->GetGpuMemoryBuffer()->GetId() - : gfx::GpuMemoryBufferId(frame->DmabufFds()[0].get()); - - scoped_refptr va_surface; - if (!base::Contains(allocated_va_surfaces_, frame_id)) { - scoped_refptr pixmap = - CreateNativePixmapDmaBuf(frame.get()); - if (!pixmap) { - LOG(ERROR) << "Failed to create NativePixmap from VideoFrame"; - SetState(State::kError); - return nullptr; - } + scoped_refptr pixmap = + CreateNativePixmapDmaBuf(frame.get()); + if (!pixmap) { + LOG(ERROR) << "Failed to create NativePixmap from VideoFrame"; + SetState(State::kError); + return nullptr; + } - va_surface = vaapi_wrapper_->CreateVASurfaceForPixmap(std::move(pixmap)); - if (!va_surface || va_surface->id() == VA_INVALID_ID) { - LOG(ERROR) << "Failed to create VASurface from VideoFrame"; - SetState(State::kError); - return nullptr; - } + // Create VASurface from the native pixmap. + scoped_refptr va_surface = + vaapi_wrapper_->CreateVASurfaceForPixmap(std::move(pixmap)); - allocated_va_surfaces_[frame_id] = va_surface; - } else { - va_surface = allocated_va_surfaces_[frame_id]; - DCHECK_EQ(frame->coded_size(), va_surface->size()); + if (!va_surface || va_surface->id() == VA_INVALID_ID) { + LOG(ERROR) << "Failed to create VASurface from VideoFrame"; + SetState(State::kError); + return nullptr; } // Store the mapping between surface and video frame, so we know which video @@ -360,12 +327,14 @@ scoped_refptr VaapiVideoDecoder::CreateSurface() { output_frames_[surface_id] = frame; // When the decoder is done using the frame for output or reference, it will - // drop its reference to the surface. We can then safely remove the associated - // video frame from |output_frames_|. To be notified when this happens we wrap - // the surface in another surface with ReleaseVideoFrame() as destruction - // observer. - VASurface::ReleaseCB release_frame_cb = - base::BindOnce(&VaapiVideoDecoder::ReleaseVideoFrame, weak_this_); + // drop its reference to the surface. We can then safely destroy the surface + // and remove the associated video frame from |output_frames_|. To be notified + // when this happens we wrap the surface in another surface that calls + // ReleaseFrame() on destruction. The |va_surface| object is bound to the + // destruction callback to keep it alive, since the associated VAAPI surface + // will be automatically destroyed when we drop the reference. + VASurface::ReleaseCB release_frame_cb = base::BindOnce( + &VaapiVideoDecoder::ReleaseFrame, weak_this_, std::move(va_surface)); return new VASurface(surface_id, frame->layout().coded_size(), GetVaFormatForVideoCodecProfile(profile_), @@ -447,11 +416,7 @@ void VaapiVideoDecoder::ApplyResolutionChange() { } // All pending decode operations will be completed before triggering a - // resolution change, so we can safely DestroyContext() here; that, in turn, - // allows for clearing the |allocated_va_surfaces_|. - vaapi_wrapper_->DestroyContext(); - allocated_va_surfaces_.clear(); - + // resolution change, so we can safely destroy the context here. if (profile_ != decoder_->GetProfile()) { // When a profile is changed, we need to re-initialize VaapiWrapper. profile_ = decoder_->GetProfile(); @@ -464,6 +429,8 @@ void VaapiVideoDecoder::ApplyResolutionChange() { } decoder_delegate_->set_vaapi_wrapper(new_vaapi_wrapper.get()); vaapi_wrapper_ = std::move(new_vaapi_wrapper); + } else { + vaapi_wrapper_->DestroyContext(); } vaapi_wrapper_->CreateContext(pic_size); @@ -480,7 +447,9 @@ void VaapiVideoDecoder::ApplyResolutionChange() { } } -void VaapiVideoDecoder::ReleaseVideoFrame(VASurfaceID surface_id) { +void VaapiVideoDecoder::ReleaseFrame(scoped_refptr va_surface, + VASurfaceID surface_id) { + DCHECK_EQ(va_surface->id(), surface_id); DVLOGF(4); DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); diff --git a/media/gpu/vaapi/vaapi_video_decoder.h b/media/gpu/vaapi/vaapi_video_decoder.h index 64dd329a0e0d2a..9390bdcc44a43a 100644 --- a/media/gpu/vaapi/vaapi_video_decoder.h +++ b/media/gpu/vaapi/vaapi_video_decoder.h @@ -15,7 +15,6 @@ #include "base/containers/mru_cache.h" #include "base/containers/queue.h" -#include "base/containers/small_map.h" #include "base/macros.h" #include "base/memory/scoped_refptr.h" #include "base/memory/weak_ptr.h" @@ -30,7 +29,6 @@ #include "media/video/supported_video_decoder_config.h" #include "ui/gfx/geometry/rect.h" #include "ui/gfx/geometry/size.h" -#include "ui/gfx/gpu_memory_buffer.h" namespace media { @@ -105,11 +103,13 @@ class VaapiVideoDecoder : public DecoderInterface, void ClearDecodeTaskQueue(DecodeStatus status); // Releases the local reference to the VideoFrame associated with the - // specified |surface_id| on the decoder thread. This is called when - // |decoder_| has outputted the VideoFrame and stopped using it as a - // reference frame. Note that this doesn't mean the frame can be reused - // immediately, as it might still be used by the client. - void ReleaseVideoFrame(VASurfaceID surface_id); + // specified |surface_id| on the decoder thread. This is called when the last + // reference to the associated VASurface has been released, which happens when + // |decoder_| outputted the video frame, or stopped using it as a reference + // frame. Note that this doesn't mean the frame can be reused immediately, as + // it might still be used by the client. + void ReleaseFrame(scoped_refptr va_surface, + VASurfaceID surface_id); // Callback for |frame_pool_| to notify of available resources. void NotifyFrameAvailable(); @@ -159,15 +159,6 @@ class VaapiVideoDecoder : public DecoderInterface, // The list of frames currently used as output buffers or reference frames. std::map> output_frames_; - // VASurfaces are created via importing |frame_pool_| resources into libva in - // CreateSurface(). The following map keeps those VASurfaces for reuse - // according to the expectations of libva vaDestroySurfaces(): "Surfaces can - // only be destroyed after all contexts using these surfaces have been - // destroyed." - // TODO(crbug.com/1040291): remove this keep-alive when using SharedImages. - base::small_map>> - allocated_va_surfaces_; - // Platform and codec specific video decoder. std::unique_ptr decoder_; scoped_refptr vaapi_wrapper_;