external/libgav1: backport sequence header fixes | DO NOT MERGE
cl/289966078: Move initial_display_delay out of OperatingParamet
cl/290784565: Handle a change of sequence header parameters.
cl/291222461: Disallow change of sequence header during a frame.
Together these provide additional coverage for the previous subsampling
fix [1], additionally adding coverage for bitdepth changes mid-frame. As
a side effect they also prevent the file from b/147491764 from
progressing to the original point of failure [2].
[1] ce9b99c external/libgav1: backport prediction subsampling fix
[2] 22ec187 external/libgav1: backport prediction border extension fix
Bug: b/147491773,b/147491764
Change-Id: I3f949e17c12990fc313522d9b2633509ee4e59a2
Test: upstream test vectors, repro test cases
(cherry picked from commit 760fb0d6f0766ca38b19ef57f8f9229aaeccca07)
diff --git a/README.version b/README.version
index 65e27d6..8a3537a 100644
--- a/README.version
+++ b/README.version
@@ -5,3 +5,6 @@
- ab3390a external/libgav1,cosmetics: add license headers
- backport cl/281117442: Fully use the frame border for reference block.
- backport cl/289984918: convolve: Use the correct subsampling for ref frames
+- backport cl/289966078: Move initial_display_delay out of OperatingParamet
+- backport cl/290784565: Handle a change of sequence header parameters.
+- backport cl/291222461: Disallow change of sequence header during a frame.
diff --git a/libgav1/src/decoder_impl.cc b/libgav1/src/decoder_impl.cc
index 9448247..11f2682 100644
--- a/libgav1/src/decoder_impl.cc
+++ b/libgav1/src/decoder_impl.cc
@@ -50,6 +50,27 @@
} // namespace
+void DecoderState::UpdateReferenceFrames(int refresh_frame_flags) {
+ for (int ref_index = 0, mask = refresh_frame_flags; mask != 0;
+ ++ref_index, mask >>= 1) {
+ if ((mask & 1) != 0) {
+ reference_valid[ref_index] = true;
+ reference_frame_id[ref_index] = current_frame_id;
+ reference_frame[ref_index] = current_frame;
+ reference_order_hint[ref_index] = order_hint;
+ }
+ }
+}
+
+void DecoderState::ClearReferenceFrames() {
+ reference_valid = {};
+ reference_frame_id = {};
+ reference_order_hint = {};
+ for (int ref_index = 0; ref_index < kNumReferenceFrameTypes; ++ref_index) {
+ reference_frame[ref_index] = nullptr;
+ }
+}
+
// static
StatusCode DecoderImpl::Create(const DecoderSettings* settings,
std::unique_ptr<DecoderImpl>* output) {
@@ -173,7 +194,7 @@
return status;
}
}
- UpdateReferenceFrames(obu->frame_header().refresh_frame_flags);
+ state_.UpdateReferenceFrames(obu->frame_header().refresh_frame_flags);
if (obu->frame_header().show_frame ||
obu->frame_header().show_existing_frame) {
if (displayable_frame != nullptr) {
@@ -195,11 +216,11 @@
obu->frame_header().refresh_frame_flags == 0) {
// If show_existing_frame is true, then the current frame is a
// previously saved reference frame. If refresh_frame_flags is
- // nonzero, then the UpdateReferenceFrames() call above has saved the
- // current frame as a reference frame. Therefore, if both of these
- // conditions are false, then the current frame is not saved as a
- // reference frame. displayable_frame should hold the only reference
- // to the current frame.
+ // nonzero, then the state_.UpdateReferenceFrames() call above has
+ // saved the current frame as a reference frame. Therefore, if both
+ // of these conditions are false, then the current frame is not
+ // saved as a reference frame. displayable_frame should hold the
+ // only reference to the current frame.
assert(displayable_frame.use_count() == 1);
// Add film grain noise in place.
film_grain_frame = displayable_frame;
@@ -665,16 +686,4 @@
}
}
-void DecoderImpl::UpdateReferenceFrames(int refresh_frame_flags) {
- for (int ref_index = 0, mask = refresh_frame_flags; mask != 0;
- ++ref_index, mask >>= 1) {
- if ((mask & 1) != 0) {
- state_.reference_valid[ref_index] = true;
- state_.reference_frame_id[ref_index] = state_.current_frame_id;
- state_.reference_frame[ref_index] = state_.current_frame;
- state_.reference_order_hint[ref_index] = state_.order_hint;
- }
- }
-}
-
} // namespace libgav1
diff --git a/libgav1/src/decoder_impl.h b/libgav1/src/decoder_impl.h
index 4e4c6b5..e58a0da 100644
--- a/libgav1/src/decoder_impl.h
+++ b/libgav1/src/decoder_impl.h
@@ -41,6 +41,13 @@
};
struct DecoderState {
+ // Section 7.20. Updates frames in the reference_frame array with
+ // current_frame, based on the refresh_frame_flags bitmask.
+ void UpdateReferenceFrames(int refresh_frame_flags);
+
+ // Clears all the reference frames.
+ void ClearReferenceFrames();
+
ObuSequenceHeader sequence_header = {};
// If true, sequence_header is valid.
bool has_sequence_header = false;
@@ -64,8 +71,8 @@
//
// NOTE: When show_existing_frame is false, it is often more convenient to
// just use the order_hint field of the frame header as OrderHint. So this
- // field is mainly used to update the state_.reference_order_hint array in
- // DecoderImpl::UpdateReferenceFrames().
+ // field is mainly used to update the reference_order_hint array in
+ // UpdateReferenceFrames().
uint8_t order_hint = 0;
// reference_frame_sign_bias[i] (a boolean) specifies the intended direction
// of the motion vector in time for each reference frame.
@@ -118,9 +125,6 @@
// is handled in Tile::DecodeBlock().
void SetCurrentFrameSegmentationMap(const ObuFrameHeader& frame_header,
const SegmentationMap* prev_segment_ids);
- // Section 7.20. Updates frames in the state_.reference_frame array with
- // state_.current_frame, based on the refresh_frame_flags bitmask.
- void UpdateReferenceFrames(int refresh_frame_flags);
Queue<EncodedFrame> encoded_frames_;
DecoderState state_;
diff --git a/libgav1/src/obu_parser.cc b/libgav1/src/obu_parser.cc
index 644f1a5..ca251fa 100644
--- a/libgav1/src/obu_parser.cc
+++ b/libgav1/src/obu_parser.cc
@@ -4,6 +4,7 @@
#include <array>
#include <cassert>
#include <climits>
+#include <cstddef>
#include <cstdint>
#include <cstring>
@@ -60,6 +61,15 @@
} // namespace
+bool ObuSequenceHeader::ParametersChanged(const ObuSequenceHeader& old) const {
+ // Note that the operating_parameters field is not compared per Section 7.5:
+ // Within a particular coded video sequence, the contents of
+ // sequence_header_obu must be bit-identical each time the sequence header
+ // appears except for the contents of operating_parameters_info.
+ return memcmp(this, &old,
+ offsetof(ObuSequenceHeader, operating_parameters)) != 0;
+}
+
// Macros to avoid repeated error checks in the parser code.
#define OBU_LOG_AND_RETURN_FALSE \
do { \
@@ -247,7 +257,7 @@
return true;
}
-bool ObuParser::ParseSequenceHeader() {
+bool ObuParser::ParseSequenceHeader(bool seen_frame_header) {
ObuSequenceHeader sequence_header = {};
int64_t scratch;
OBU_READ_LITERAL_OR_FAIL(3);
@@ -307,8 +317,7 @@
OBU_READ_BIT_OR_FAIL;
if (static_cast<bool>(scratch)) {
OBU_READ_LITERAL_OR_FAIL(4);
- sequence_header.operating_parameters.initial_display_delay[i] =
- 1 + scratch;
+ sequence_header.initial_display_delay[i] = 1 + scratch;
}
}
}
@@ -400,7 +409,17 @@
if (!ParseColorConfig(&sequence_header)) return false;
OBU_READ_BIT_OR_FAIL;
sequence_header.film_grain_params_present = static_cast<bool>(scratch);
- // TODO(wtc): Compare new sequence header with old sequence header.
+ // Compare new sequence header with old sequence header.
+ if (has_sequence_header_ &&
+ sequence_header.ParametersChanged(sequence_header_)) {
+ // Between the frame header OBU and the last tile group OBU of the frame,
+ // do not allow the sequence header to change.
+ if (seen_frame_header) {
+ LIBGAV1_DLOG(ERROR, "Sequence header changed in the middle of a frame.");
+ return false;
+ }
+ decoder_state_.ClearReferenceFrames();
+ }
sequence_header_ = sequence_header;
has_sequence_header_ = true;
// Section 6.4.1: It is a requirement of bitstream conformance that if
@@ -1944,6 +1963,20 @@
}
}
}
+ // Validate frame_header_.primary_reference_frame.
+ if (frame_header_.primary_reference_frame != kPrimaryReferenceNone) {
+ const int index =
+ frame_header_
+ .reference_frame_index[frame_header_.primary_reference_frame];
+ if (decoder_state_.reference_frame[index] == nullptr) {
+ LIBGAV1_DLOG(ERROR,
+ "primary_ref_frame is %d but ref_frame_idx[%d] (%d) is "
+ "not a decoded frame.",
+ frame_header_.primary_reference_frame,
+ frame_header_.primary_reference_frame, index);
+ return false;
+ }
+ }
if (frame_header_.frame_size_override_flag &&
!frame_header_.error_resilient_mode) {
// Section 5.9.7.
@@ -2296,7 +2329,7 @@
case kObuTemporalDelimiter:
break;
case kObuSequenceHeader:
- if (!ParseSequenceHeader()) {
+ if (!ParseSequenceHeader(seen_frame_header)) {
LIBGAV1_DLOG(ERROR, "Failed to parse SequenceHeader OBU.");
return false;
}
diff --git a/libgav1/src/obu_parser.h b/libgav1/src/obu_parser.h
index 9d972b1..e973433 100644
--- a/libgav1/src/obu_parser.h
+++ b/libgav1/src/obu_parser.h
@@ -5,6 +5,7 @@
#include <cstddef>
#include <cstdint>
#include <memory>
+#include <type_traits>
#include "src/decoder_buffer.h"
#include "src/dsp/common.h"
@@ -166,13 +167,25 @@
};
struct OperatingParameters {
- uint8_t initial_display_delay[kMaxOperatingPoints];
uint32_t decoder_buffer_delay[kMaxOperatingPoints];
uint32_t encoder_buffer_delay[kMaxOperatingPoints];
bool low_delay_mode_flag[kMaxOperatingPoints];
};
struct ObuSequenceHeader {
+ // Section 7.5:
+ // Within a particular coded video sequence, the contents of
+ // sequence_header_obu must be bit-identical each time the sequence header
+ // appears except for the contents of operating_parameters_info. A new
+ // coded video sequence is required if the sequence header parameters
+ // change.
+ //
+ // IMPORTANT: ParametersChanged() is implemented with a memcmp() call. For
+ // this to work, this object and the |old| object must be initialized with
+ // an empty brace-enclosed list, which initializes any padding to zero bits.
+ // See https://en.cppreference.com/w/cpp/language/zero_initialization.
+ bool ParametersChanged(const ObuSequenceHeader& old) const;
+
BitstreamProfile profile;
bool still_picture;
bool reduced_still_picture_header;
@@ -213,11 +226,30 @@
bool decoder_model_info_present_flag;
DecoderModelInfo decoder_model_info;
bool decoder_model_present_for_operating_point[kMaxOperatingPoints];
- OperatingParameters operating_parameters;
bool initial_display_delay_present_flag;
+ uint8_t initial_display_delay[kMaxOperatingPoints];
bool film_grain_params_present;
+
+ // IMPORTANT: the operating_parameters member must be at the end of the
+ // struct so that ParametersChanged() can be implemented with a memcmp()
+ // call.
+ OperatingParameters operating_parameters;
};
+// Verify it is safe to use offsetof with ObuSequenceHeader and to use memcmp
+// to compare two ObuSequenceHeader objects.
+static_assert(std::is_standard_layout<ObuSequenceHeader>::value, "");
+// Verify operating_parameters is the last member of ObuSequenceHeader. The
+// second assertion assumes that ObuSequenceHeader has no padding after the
+// operating_parameters field. The first assertion is a sufficient condition
+// for ObuSequenceHeader to have no padding after the operating_parameters
+// field.
+static_assert(alignof(ObuSequenceHeader) == alignof(OperatingParameters), "");
+static_assert(sizeof(ObuSequenceHeader) ==
+ offsetof(ObuSequenceHeader, operating_parameters) +
+ sizeof(OperatingParameters),
+ "");
+
// Loop filter parameters:
//
// If level[0] and level[1] are both equal to 0, the loop filter process is
@@ -477,7 +509,7 @@
bool ParseDecoderModelInfo(ObuSequenceHeader* sequence_header); // 5.5.4.
bool ParseOperatingParameters(ObuSequenceHeader* sequence_header,
int index); // 5.5.5.
- bool ParseSequenceHeader(); // 5.5.1.
+ bool ParseSequenceHeader(bool seen_frame_header); // 5.5.1.
bool ParseFrameParameters(); // 5.9.2, 5.9.7 and 5.9.10.
void MarkInvalidReferenceFrames(); // 5.9.4.
bool ParseFrameSizeAndRenderSize(); // 5.9.5 and 5.9.6.
