Skip to content

Commit

Permalink
media: Address some leftover comments from CL:3942434.
Browse files Browse the repository at this point in the history
This CL addresses leftover work from [1]. Specifically:

- In v4l2_video_decoder_delegate_av1.cc::SetupFrameParams(), the block
  that fills out v4l2_frame_params.order_hints needs to be guarded by
  !libgav1::IsIntraFrame(frame_header->frame_type) instead of
  frame_header.frame_type != libgav1::kFrameKey. The reasons are that a)
  the OrderHints variable in the spec is only filled out for non-intra
  frames (not just for non-key frames); and b) if the frame is
  intra-only or key, frame_header.reference_frame_index may be
  uninitialized [2] so it's unsafe to use it.

- The static_assert that checks the size of the
  v4l2_frame_params.order_hints array is moved closer to the loop and is
  made to compare against libgav1::kNumInterReferenceFrameTypes + 1
  instead of libgav1::kNumReferenceFrameTypes to make it clearer that
  the static_assert is there to document/check that the indices accessed
  by the subsequent loop are safe to access (the loop uses
  libgav1::kNumInterReferenceFrameTypes).

We also make corresponding changes in the standalone
v4l2_stateless_decoder.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/3942434/comment/70e35d4c_e7698200/
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/libgav1/src/src/utils/types.h;l=497-499;drc=c55bc9dd7f26ec3b1de0673caa1510bfa3094b3f

Bug: b:248602457
Test: tast run ${IP} video.*av1* on a cherry device
Change-Id: I41b56576f2dfe2c9e7dcf0affa9ae2dc43e544e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4112326
Auto-Submit: Andres Calderon Jaramillo <andrescj@chromium.org>
Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org>
Commit-Queue: Steve Cho <stevecho@chromium.org>
Reviewed-by: Steve Cho <stevecho@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1084090}
  • Loading branch information
andrescj-chromium authored and Chromium LUCI CQ committed Dec 16, 2022
1 parent 2265678 commit d8e4ec6
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 9 deletions.
4 changes: 2 additions & 2 deletions media/gpu/v4l2/test/av1_decoder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -837,9 +837,9 @@ void Av1Decoder::SetupFrameParams(
// The first slot in |order_hints| is reserved for intra frame, so it is not
// used and will always be 0.
static_assert(std::size(decltype(v4l2_frame_params->order_hints){}) ==
libgav1::kNumReferenceFrameTypes,
libgav1::kNumInterReferenceFrameTypes + 1,
"Invalid size of |order_hints| array");
if (frm_header.frame_type != libgav1::kFrameKey) {
if (!libgav1::IsIntraFrame(frm_header.frame_type)) {
for (size_t i = 0; i < libgav1::kNumInterReferenceFrameTypes; i++) {
v4l2_frame_params->order_hints[i + 1] =
ref_order_hint_[frm_header.reference_frame_index[i]];
Expand Down
13 changes: 6 additions & 7 deletions media/gpu/v4l2/v4l2_video_decoder_delegate_av1.cc
Original file line number Diff line number Diff line change
Expand Up @@ -617,22 +617,21 @@ struct v4l2_ctrl_av1_frame SetupFrameParams(
frame_header.buffer_removal_time);
v4l2_frame_params.refresh_frame_flags = frame_header.refresh_frame_flags;

static_assert(std::size(decltype(v4l2_frame_params.order_hints){}) ==
libgav1::kNumReferenceFrameTypes,
"Invalid size of |order_hints| array");

// |reference_frame_index| indicates which reference frame slot is used for
// different reference frame types: L(1), L2(2), L3(3), G(4), BWD(5), A2(6),
// A(7). As |ref_frames[i]| is a |AV1Picture| with frame header info, we can
// extract |order_hint| directly for each reference frame type instead of
// maintaining |RefOrderHint| array in the AV1 spec.
if (frame_header.frame_type != libgav1::kFrameKey) {
static_assert(std::size(decltype(v4l2_frame_params.order_hints){}) ==
libgav1::kNumInterReferenceFrameTypes + 1,
"Invalid size of |order_hints| array");
if (!libgav1::IsIntraFrame(frame_header.frame_type)) {
for (size_t i = 0; i < libgav1::kNumInterReferenceFrameTypes; ++i) {
const int8_t reference_frame_index =
frame_header.reference_frame_index[i];

// TODO(b/253676775): Add safety check to guarantee DCHECK()s
// in AV1Decoder::CheckAndCleanUpReferenceFrames()
// The DCHECK()s are guaranteed by
// AV1Decoder::CheckAndCleanUpReferenceFrames().
DCHECK_GE(reference_frame_index, 0);
DCHECK_LT(reference_frame_index, libgav1::kNumReferenceFrameTypes);
DCHECK(ref_frames[reference_frame_index]);
Expand Down

0 comments on commit d8e4ec6

Please sign in to comment.