Skip to content

Commit

Permalink
Don't parallelize YUV-to-RGB conversion of AVIF
Browse files Browse the repository at this point in the history
Merge to release branch 5481 for Chrome M110.

Do not parallelize the YUV-to-RGB conversion of AVIF images. This avoids
a potential temporary deadlock of base::ThreadPool if
blink::AVIFImageDecoder::DecodeFrameBufferAtIndex() is called from
base::ThreadPool to decode many large images in parallel.

(cherry picked from commit 07383b7)

Bug: 1402841
Change-Id: I1a3df1faafde5cd04d3f9860e57de5c643a5a0b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4128587
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Wan-Teh Chang <wtc@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1089066}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4148834
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Auto-Submit: Wan-Teh Chang <wtc@google.com>
Cr-Commit-Position: refs/branch-heads/5481@{#193}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
wantehchang authored and Chromium LUCI CQ committed Jan 10, 2023
1 parent 8f74343 commit d0fd1dd
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 11 deletions.
20 changes: 12 additions & 8 deletions media/renderers/paint_canvas_video_renderer.cc
Expand Up @@ -358,6 +358,14 @@ libyuv::FilterMode ToLibyuvFilterMode(
}
}

size_t NumConvertVideoFrameToRGBPixelsTasks(const VideoFrame* video_frame) {
constexpr size_t kTaskBytes = 1024 * 1024; // 1 MiB
const size_t frame_size = VideoFrame::AllocationSize(
video_frame->format(), video_frame->visible_rect().size());
const size_t n_tasks = std::max<size_t>(1, frame_size / kTaskBytes);
return std::min<size_t>(n_tasks, base::SysInfo::NumberOfProcessors());
}

void ConvertVideoFrameToRGBPixelsTask(const VideoFrame* video_frame,
void* rgb_pixels,
size_t row_bytes,
Expand Down Expand Up @@ -1335,7 +1343,8 @@ void PaintCanvasVideoRenderer::ConvertVideoFrameToRGBPixels(
void* rgb_pixels,
size_t row_bytes,
bool premultiply_alpha,
FilterMode filter) {
FilterMode filter,
bool disable_threading) {
if (!video_frame->IsMappable()) {
NOTREACHED() << "Cannot extract pixels from non-CPU frame formats.";
return;
Expand Down Expand Up @@ -1374,13 +1383,8 @@ void PaintCanvasVideoRenderer::ConvertVideoFrameToRGBPixels(
break;
}

constexpr size_t kTaskBytes = 1024 * 1024; // 1 MiB
const size_t n_tasks = std::min<size_t>(
std::max<size_t>(
1, VideoFrame::AllocationSize(video_frame->format(),
video_frame->visible_rect().size()) /
kTaskBytes),
base::SysInfo::NumberOfProcessors());
const size_t n_tasks =
disable_threading ? 1 : NumConvertVideoFrameToRGBPixelsTasks(video_frame);
base::WaitableEvent event;
base::RepeatingClosure barrier = base::BarrierClosure(
n_tasks,
Expand Down
10 changes: 8 additions & 2 deletions media/renderers/paint_canvas_video_renderer.h
Expand Up @@ -92,7 +92,12 @@ class MEDIA_EXPORT PaintCanvasVideoRenderer {
// indicates whether the R, G, B samples in |rgb_pixels| should be multiplied
// by alpha. |filter| specifies the chroma upsampling filter used for pixel
// formats with chroma subsampling. If chroma planes in the pixel format are
// not subsampled, |filter| is ignored.
// not subsampled, |filter| is ignored. |disable_threading| indicates whether
// this method should convert |video_frame| without posting any tasks to
// base::ThreadPool, regardless of the frame size. If this method is called
// from a task running in base::ThreadPool, setting |disable_threading| to
// true can avoid a potential temporary deadlock of base::ThreadPool. See
// crbug.com/1402841.
//
// NOTE: If |video_frame| doesn't have an alpha plane, all the A samples in
// |rgb_pixels| will be 255 (equivalent to an alpha of 1.0) and therefore the
Expand All @@ -102,7 +107,8 @@ class MEDIA_EXPORT PaintCanvasVideoRenderer {
void* rgb_pixels,
size_t row_bytes,
bool premultiply_alpha = true,
FilterMode filter = kFilterNone);
FilterMode filter = kFilterNone,
bool disable_threading = false);

// The output format that ConvertVideoFrameToRGBPixels will write.
static viz::ResourceFormat GetRGBPixelsOutputFormat();
Expand Down
Expand Up @@ -1150,7 +1150,8 @@ bool AVIFImageDecoder::RenderImage(const avifImage* image,
// https://bugs.chromium.org/p/libyuv/issues/detail?id=845
media::PaintCanvasVideoRenderer::ConvertVideoFrameToRGBPixels(
frame.get(), rgba_8888, frame->visible_rect().width() * 4,
premultiply_alpha, media::PaintCanvasVideoRenderer::kFilterBilinear);
premultiply_alpha, media::PaintCanvasVideoRenderer::kFilterBilinear,
/*disable_threading=*/true);

if (save_top_row) {
base::ranges::copy(previous_last_decoded_row_, rgba_8888);
Expand Down
Expand Up @@ -9,8 +9,13 @@
#include <ostream>
#include <vector>

#include "base/barrier_closure.h"
#include "base/bit_cast.h"
#include "base/functional/bind.h"
#include "base/strings/stringprintf.h"
#include "base/synchronization/waitable_event.h"
#include "base/task/thread_pool.h"
#include "base/test/task_environment.h"
#include "media/media_buildflags.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/renderer/platform/image-decoders/image_decoder_test_helpers.h"
Expand Down Expand Up @@ -725,6 +730,24 @@ void TestYUVRed(const char* file_name,
EXPECT_NEAR(decoded_pixel.z(), 0, kMinError); // B
}

void DecodeTask(const SharedBuffer* data, base::RepeatingClosure* done) {
std::unique_ptr<ImageDecoder> decoder = CreateAVIFDecoder();

scoped_refptr<SharedBuffer> data_copy = SharedBuffer::Create();
data_copy->Append(*data);
decoder->SetData(std::move(data_copy), true);

EXPECT_TRUE(decoder->IsSizeAvailable());
EXPECT_FALSE(decoder->Failed());
EXPECT_EQ(decoder->FrameCount(), 1u);
ImageFrame* frame = decoder->DecodeFrameBufferAtIndex(0);
ASSERT_TRUE(frame);
EXPECT_EQ(ImageFrame::kFrameComplete, frame->GetStatus());
EXPECT_FALSE(decoder->Failed());

done->Run();
}

} // namespace

TEST(AnimatedAVIFTests, ValidImages) {
Expand Down Expand Up @@ -1029,6 +1052,43 @@ TEST(StaticAVIFTests, IncrementalDecoding) {
}
}

// Reproduces crbug.com/1402841. Decodes a large AVIF image 104 times in
// parallel from base::ThreadPool. Should not cause temporary deadlock of
// base::ThreadPool.
TEST(StaticAVIFTests, ParallelDecoding) {
// The base::test::TaskEnvironment constructor creates a base::ThreadPool
// instance with 4 foreground threads. The number 4 comes from the
// test::TaskEnvironment::kNumForegroundThreadPoolThreads constant.
base::test::TaskEnvironment task_environment;

// This test image is fast to decode (all neutral gray pixels) and its
// allocation size is large enough to cause
// media::PaintCanvasVideoRenderer::ConvertVideoFrameToRGBPixels() to pick
// n_tasks > 1 if AVIFImageDecoder did not pass disable_threading=true to it.
scoped_refptr<SharedBuffer> data =
ReadFile("/images/resources/avif/gray1024x704.avif");
ASSERT_TRUE(data.get());

// Task timeout in tests is 30 seconds (see https://crrev.com/c/1949028).
// Four blocking tasks cause a temporary deadlock (1.2 seconds) of
// base::ThreadPool, so we need at least 30 / 1.2 * 4 = 100 decodes for the
// test to time out without the bug fix. We add a margin of 4 decodes, i.e.,
// (30 / 1.2 + 1) * 4 = 104.
const size_t n_decodes = 104;
base::WaitableEvent event;
base::RepeatingClosure barrier = base::BarrierClosure(
n_decodes,
base::BindOnce(&base::WaitableEvent::Signal, base::Unretained(&event)));

for (size_t i = 0; i < n_decodes; ++i) {
base::ThreadPool::PostTask(
FROM_HERE,
base::BindOnce(DecodeTask, base::Unretained(data.get()), &barrier));
}

event.Wait();
}

TEST(StaticAVIFTests, AlphaHasNoIspeProperty) {
std::unique_ptr<ImageDecoder> decoder = CreateAVIFDecoder();
decoder->SetData(ReadFile("/images/resources/avif/green-no-alpha-ispe.avif"),
Expand Down
Binary file not shown.

0 comments on commit d0fd1dd

Please sign in to comment.