Skip to content

Commit

Permalink
Revert "Reland "RELAND 2: media/gpu/vaapi_video_decoder: keep allocat…
Browse files Browse the repository at this point in the history 
…ed VASurfaces alive""

This reverts commit 49422e8.

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 02aec84
> 
> 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 <mcasas@chromium.org>
> > Commit-Queue: Miguel Casas <mcasas@chromium.org>
> > Cr-Original-Commit-Position: refs/heads/master@{#795026}
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2359734
> > Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org>
> > Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org>
> > Auto-Submit: Miguel Casas <mcasas@chromium.org>
> > 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 <sidereal@google.com>
> Commit-Queue: Fergus Dall <sidereal@google.com>
> 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 <guidou@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799038}
  • Loading branch information
Guido Urdaneta authored and Commit Bot committed Aug 18, 2020
1 parent b352a52 commit f97ca58
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 75 deletions.
87 changes: 28 additions & 59 deletions media/gpu/vaapi/vaapi_video_decoder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -319,36 +300,22 @@ scoped_refptr<VASurface> 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<VASurface> va_surface;
if (!base::Contains(allocated_va_surfaces_, frame_id)) {
scoped_refptr<gfx::NativePixmap> pixmap =
CreateNativePixmapDmaBuf(frame.get());
if (!pixmap) {
LOG(ERROR) << "Failed to create NativePixmap from VideoFrame";
SetState(State::kError);
return nullptr;
}
scoped_refptr<gfx::NativePixmap> 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<VASurface> 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
Expand All @@ -360,12 +327,14 @@ scoped_refptr<VASurface> 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_),
Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -480,7 +447,9 @@ void VaapiVideoDecoder::ApplyResolutionChange() {
}
}

void VaapiVideoDecoder::ReleaseVideoFrame(VASurfaceID surface_id) {
void VaapiVideoDecoder::ReleaseFrame(scoped_refptr<VASurface> va_surface,
VASurfaceID surface_id) {
DCHECK_EQ(va_surface->id(), surface_id);
DVLOGF(4);
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

Expand Down
23 changes: 7 additions & 16 deletions media/gpu/vaapi/vaapi_video_decoder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 {

Expand Down Expand Up @@ -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<VASurface> va_surface,
VASurfaceID surface_id);
// Callback for |frame_pool_| to notify of available resources.
void NotifyFrameAvailable();

Expand Down Expand Up @@ -159,15 +159,6 @@ class VaapiVideoDecoder : public DecoderInterface,
// The list of frames currently used as output buffers or reference frames.
std::map<VASurfaceID, scoped_refptr<VideoFrame>> 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<std::map<gfx::GpuMemoryBufferId, scoped_refptr<VASurface>>>
allocated_va_surfaces_;

// Platform and codec specific video decoder.
std::unique_ptr<AcceleratedVideoDecoder> decoder_;
scoped_refptr<VaapiWrapper> vaapi_wrapper_;
Expand Down

0 comments on commit f97ca58

Please sign in to comment.