| Message ID | 20240323173735.26224-3-ffmpeg@haasn.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/6] avutil/dovi_meta: add AVDOVIDataMapping.nlq_pivots | expand |
| Context | Check | Description |
|---|---|---|
| andriy/configure_x86 | warning | Failed to apply patch |
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
Niklas Haas: > From: Niklas Haas <git@haasn.dev> > > This ensures that `gb` in the following section is fully byte-aligned, > points at the start of the actual RPU, and ends on the CRC terminator. > > This is important for both calculation of the CRC, as well as dovi > extension block parsing (which aligns to byte boundaries in various > places). > --- > libavcodec/dovi_rpu.c | 48 +++++++++++++++++++++++++++++++++++-------- > libavcodec/dovi_rpu.h | 2 ++ > 2 files changed, 41 insertions(+), 9 deletions(-) > > diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c > index c84a942f476..24a1cdf39a8 100644 > --- a/libavcodec/dovi_rpu.c > +++ b/libavcodec/dovi_rpu.c > @@ -23,6 +23,7 @@ > > #include "libavutil/buffer.h" > > +#include "avcodec.h" What is that used for? > #include "dovi_rpu.h" > #include "golomb.h" > #include "get_bits.h" > @@ -45,6 +46,7 @@ void ff_dovi_ctx_unref(DOVIContext *s) > { > for (int i = 0; i < FF_ARRAY_ELEMS(s->vdr); i++) > ff_refstruct_unref(&s->vdr[i]); > + av_free(s->rpu_buf); > > *s = (DOVIContext) { > .logctx = s->logctx, > @@ -202,17 +204,17 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) > DOVIVdr *vdr; > int ret; > > - uint8_t nal_prefix; > uint8_t rpu_type; > uint8_t vdr_seq_info_present; > uint8_t vdr_dm_metadata_present; > uint8_t use_prev_vdr_rpu; > uint8_t use_nlq; > uint8_t profile; > - if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) > - return ret; > > - /* Container header */ > + if (rpu_size < 5) > + goto fail; > + > + /* Container */ > if (s->dv_profile == 10 /* dav1.10 */) { > /* DV inside AV1 re-uses an EMDF container skeleton, but with fixed > * values - so we can effectively treat this as a magic byte sequence. > @@ -229,18 +231,46 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) > * discard_unknown_payload : f(1) = 1 > */ > const unsigned header_magic = 0x01be6841u; > - unsigned header, emdf_payload_size; > - header = get_bits_long(gb, 27); > - VALIDATE(header, header_magic, header_magic); > + unsigned emdf_header, emdf_payload_size, emdf_protection; > + if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) > + return ret; > + emdf_header = get_bits_long(gb, 27); > + VALIDATE(emdf_header, header_magic, header_magic); > emdf_payload_size = get_variable_bits(gb, 8); > VALIDATE(emdf_payload_size, 6, 512); > if (emdf_payload_size * 8 > get_bits_left(gb)) > return AVERROR_INVALIDDATA; > + > + /* The payload is not byte-aligned (off by *one* bit, curse Dolby), > + * so copy into a fresh buffer to preserve byte alignment of the > + * RPU struct */ > + av_fast_padded_malloc(&s->rpu_buf, &s->rpu_buf_sz, emdf_payload_size); > + if (!s->rpu_buf) > + return AVERROR(ENOMEM); > + for (int i = 0; i < emdf_payload_size; i++) > + s->rpu_buf[i] = get_bits(gb, 8); > + rpu = s->rpu_buf; > + rpu_size = emdf_payload_size; > + > + /* Validate EMDF footer */ > + emdf_protection = get_bits(gb, 5 + 12); > + VALIDATE(emdf_protection, 0x400, 0x400); > + > + if ((ret = init_get_bits8(gb, s->rpu_buf, emdf_payload_size)) < 0) > + return ret; > } else { > - nal_prefix = get_bits(gb, 8); > - VALIDATE(nal_prefix, 25, 25); > + /* NAL RBSP with prefix and trailing zeroes */ > + VALIDATE(rpu[0], 25, 25); /* NAL prefix */ > + rpu++; > + rpu_size--; > + /* Strip trailing padding bytes */ > + while (rpu_size && rpu[rpu_size - 1] == 0) > + rpu_size--; > } > > + if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) > + return ret; > + > /* RPU header */ > rpu_type = get_bits(gb, 6); > if (rpu_type != 2) { > diff --git a/libavcodec/dovi_rpu.h b/libavcodec/dovi_rpu.h > index 51c5fdbb879..506974a74bf 100644 > --- a/libavcodec/dovi_rpu.h > +++ b/libavcodec/dovi_rpu.h > @@ -49,6 +49,8 @@ typedef struct DOVIContext { > */ > struct DOVIVdr *vdr[DOVI_MAX_DM_ID+1]; ///< RefStruct references > uint8_t dv_profile; > + uint8_t *rpu_buf; ///< temporary buffer > + unsigned rpu_buf_sz; The order here introduces unnecessary padding. > > } DOVIContext; >
On Sat, 23 Mar 2024 19:02:26 +0100 Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > Niklas Haas: > > From: Niklas Haas <git@haasn.dev> > > > > This ensures that `gb` in the following section is fully byte-aligned, > > points at the start of the actual RPU, and ends on the CRC terminator. > > > > This is important for both calculation of the CRC, as well as dovi > > extension block parsing (which aligns to byte boundaries in various > > places). > > --- > > libavcodec/dovi_rpu.c | 48 +++++++++++++++++++++++++++++++++++-------- > > libavcodec/dovi_rpu.h | 2 ++ > > 2 files changed, 41 insertions(+), 9 deletions(-) > > > > diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c > > index c84a942f476..24a1cdf39a8 100644 > > --- a/libavcodec/dovi_rpu.c > > +++ b/libavcodec/dovi_rpu.c > > @@ -23,6 +23,7 @@ > > > > #include "libavutil/buffer.h" > > > > +#include "avcodec.h" > > What is that used for? av_fast_padded_malloc > > > #include "dovi_rpu.h" > > #include "golomb.h" > > #include "get_bits.h" > > @@ -45,6 +46,7 @@ void ff_dovi_ctx_unref(DOVIContext *s) > > { > > for (int i = 0; i < FF_ARRAY_ELEMS(s->vdr); i++) > > ff_refstruct_unref(&s->vdr[i]); > > + av_free(s->rpu_buf); > > > > *s = (DOVIContext) { > > .logctx = s->logctx, > > @@ -202,17 +204,17 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) > > DOVIVdr *vdr; > > int ret; > > > > - uint8_t nal_prefix; > > uint8_t rpu_type; > > uint8_t vdr_seq_info_present; > > uint8_t vdr_dm_metadata_present; > > uint8_t use_prev_vdr_rpu; > > uint8_t use_nlq; > > uint8_t profile; > > - if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) > > - return ret; > > > > - /* Container header */ > > + if (rpu_size < 5) > > + goto fail; > > + > > + /* Container */ > > if (s->dv_profile == 10 /* dav1.10 */) { > > /* DV inside AV1 re-uses an EMDF container skeleton, but with fixed > > * values - so we can effectively treat this as a magic byte sequence. > > @@ -229,18 +231,46 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) > > * discard_unknown_payload : f(1) = 1 > > */ > > const unsigned header_magic = 0x01be6841u; > > - unsigned header, emdf_payload_size; > > - header = get_bits_long(gb, 27); > > - VALIDATE(header, header_magic, header_magic); > > + unsigned emdf_header, emdf_payload_size, emdf_protection; > > + if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) > > + return ret; > > + emdf_header = get_bits_long(gb, 27); > > + VALIDATE(emdf_header, header_magic, header_magic); > > emdf_payload_size = get_variable_bits(gb, 8); > > VALIDATE(emdf_payload_size, 6, 512); > > if (emdf_payload_size * 8 > get_bits_left(gb)) > > return AVERROR_INVALIDDATA; > > + > > + /* The payload is not byte-aligned (off by *one* bit, curse Dolby), > > + * so copy into a fresh buffer to preserve byte alignment of the > > + * RPU struct */ > > + av_fast_padded_malloc(&s->rpu_buf, &s->rpu_buf_sz, emdf_payload_size); > > + if (!s->rpu_buf) > > + return AVERROR(ENOMEM); > > + for (int i = 0; i < emdf_payload_size; i++) > > + s->rpu_buf[i] = get_bits(gb, 8); > > + rpu = s->rpu_buf; > > + rpu_size = emdf_payload_size; > > + > > + /* Validate EMDF footer */ > > + emdf_protection = get_bits(gb, 5 + 12); > > + VALIDATE(emdf_protection, 0x400, 0x400); > > + > > + if ((ret = init_get_bits8(gb, s->rpu_buf, emdf_payload_size)) < 0) > > + return ret; > > } else { > > - nal_prefix = get_bits(gb, 8); > > - VALIDATE(nal_prefix, 25, 25); > > + /* NAL RBSP with prefix and trailing zeroes */ > > + VALIDATE(rpu[0], 25, 25); /* NAL prefix */ > > + rpu++; > > + rpu_size--; > > + /* Strip trailing padding bytes */ > > + while (rpu_size && rpu[rpu_size - 1] == 0) > > + rpu_size--; > > } > > > > + if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) > > + return ret; > > + > > /* RPU header */ > > rpu_type = get_bits(gb, 6); > > if (rpu_type != 2) { > > diff --git a/libavcodec/dovi_rpu.h b/libavcodec/dovi_rpu.h > > index 51c5fdbb879..506974a74bf 100644 > > --- a/libavcodec/dovi_rpu.h > > +++ b/libavcodec/dovi_rpu.h > > @@ -49,6 +49,8 @@ typedef struct DOVIContext { > > */ > > struct DOVIVdr *vdr[DOVI_MAX_DM_ID+1]; ///< RefStruct references > > uint8_t dv_profile; > > + uint8_t *rpu_buf; ///< temporary buffer > > + unsigned rpu_buf_sz; > > The order here introduces unnecessary padding. Fixed. > > > > > } DOVIContext; > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c index c84a942f476..24a1cdf39a8 100644 --- a/libavcodec/dovi_rpu.c +++ b/libavcodec/dovi_rpu.c @@ -23,6 +23,7 @@ #include "libavutil/buffer.h" +#include "avcodec.h" #include "dovi_rpu.h" #include "golomb.h" #include "get_bits.h" @@ -45,6 +46,7 @@ void ff_dovi_ctx_unref(DOVIContext *s) { for (int i = 0; i < FF_ARRAY_ELEMS(s->vdr); i++) ff_refstruct_unref(&s->vdr[i]); + av_free(s->rpu_buf); *s = (DOVIContext) { .logctx = s->logctx, @@ -202,17 +204,17 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) DOVIVdr *vdr; int ret; - uint8_t nal_prefix; uint8_t rpu_type; uint8_t vdr_seq_info_present; uint8_t vdr_dm_metadata_present; uint8_t use_prev_vdr_rpu; uint8_t use_nlq; uint8_t profile; - if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) - return ret; - /* Container header */ + if (rpu_size < 5) + goto fail; + + /* Container */ if (s->dv_profile == 10 /* dav1.10 */) { /* DV inside AV1 re-uses an EMDF container skeleton, but with fixed * values - so we can effectively treat this as a magic byte sequence. @@ -229,18 +231,46 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) * discard_unknown_payload : f(1) = 1 */ const unsigned header_magic = 0x01be6841u; - unsigned header, emdf_payload_size; - header = get_bits_long(gb, 27); - VALIDATE(header, header_magic, header_magic); + unsigned emdf_header, emdf_payload_size, emdf_protection; + if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) + return ret; + emdf_header = get_bits_long(gb, 27); + VALIDATE(emdf_header, header_magic, header_magic); emdf_payload_size = get_variable_bits(gb, 8); VALIDATE(emdf_payload_size, 6, 512); if (emdf_payload_size * 8 > get_bits_left(gb)) return AVERROR_INVALIDDATA; + + /* The payload is not byte-aligned (off by *one* bit, curse Dolby), + * so copy into a fresh buffer to preserve byte alignment of the + * RPU struct */ + av_fast_padded_malloc(&s->rpu_buf, &s->rpu_buf_sz, emdf_payload_size); + if (!s->rpu_buf) + return AVERROR(ENOMEM); + for (int i = 0; i < emdf_payload_size; i++) + s->rpu_buf[i] = get_bits(gb, 8); + rpu = s->rpu_buf; + rpu_size = emdf_payload_size; + + /* Validate EMDF footer */ + emdf_protection = get_bits(gb, 5 + 12); + VALIDATE(emdf_protection, 0x400, 0x400); + + if ((ret = init_get_bits8(gb, s->rpu_buf, emdf_payload_size)) < 0) + return ret; } else { - nal_prefix = get_bits(gb, 8); - VALIDATE(nal_prefix, 25, 25); + /* NAL RBSP with prefix and trailing zeroes */ + VALIDATE(rpu[0], 25, 25); /* NAL prefix */ + rpu++; + rpu_size--; + /* Strip trailing padding bytes */ + while (rpu_size && rpu[rpu_size - 1] == 0) + rpu_size--; } + if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) + return ret; + /* RPU header */ rpu_type = get_bits(gb, 6); if (rpu_type != 2) { diff --git a/libavcodec/dovi_rpu.h b/libavcodec/dovi_rpu.h index 51c5fdbb879..506974a74bf 100644 --- a/libavcodec/dovi_rpu.h +++ b/libavcodec/dovi_rpu.h @@ -49,6 +49,8 @@ typedef struct DOVIContext { */ struct DOVIVdr *vdr[DOVI_MAX_DM_ID+1]; ///< RefStruct references uint8_t dv_profile; + uint8_t *rpu_buf; ///< temporary buffer + unsigned rpu_buf_sz; } DOVIContext;
From: Niklas Haas <git@haasn.dev> This ensures that `gb` in the following section is fully byte-aligned, points at the start of the actual RPU, and ends on the CRC terminator. This is important for both calculation of the CRC, as well as dovi extension block parsing (which aligns to byte boundaries in various places). --- libavcodec/dovi_rpu.c | 48 +++++++++++++++++++++++++++++++++++-------- libavcodec/dovi_rpu.h | 2 ++ 2 files changed, 41 insertions(+), 9 deletions(-)