* [FFmpeg-devel] [PATCH 2/5] avcodec/bsf/hevc_mp4toannexb: Factor creating new extradata out
2024-02-18 2:41 [FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX Andreas Rheinhardt
@ 2024-02-18 2:44 ` Andreas Rheinhardt
2024-02-18 2:44 ` [FFmpeg-devel] [PATCH 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata Andreas Rheinhardt
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Andreas Rheinhardt @ 2024-02-18 2:44 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Andreas Rheinhardt
This is in preparation for the next commit.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
libavcodec/bsf/hevc_mp4toannexb.c | 64 ++++++++++++++++++++-----------
1 file changed, 41 insertions(+), 23 deletions(-)
diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c
index c0df2b79a6..a695cba370 100644
--- a/libavcodec/bsf/hevc_mp4toannexb.c
+++ b/libavcodec/bsf/hevc_mp4toannexb.c
@@ -37,38 +37,32 @@ typedef struct HEVCBSFContext {
int extradata_parsed;
} HEVCBSFContext;
-static int hevc_extradata_to_annexb(AVBSFContext *ctx)
+static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
+ uint8_t **new_extradatap,
+ size_t *new_extradata_sizep)
{
- GetByteContext gb;
- int length_size, num_arrays, i, j;
- int ret = 0;
-
+ int num_arrays = bytestream2_get_byte(gb);
uint8_t *new_extradata = NULL;
- size_t new_extradata_size = 0;
-
- bytestream2_init(&gb, ctx->par_in->extradata, ctx->par_in->extradata_size);
-
- bytestream2_skip(&gb, 21);
- length_size = (bytestream2_get_byte(&gb) & 3) + 1;
- num_arrays = bytestream2_get_byte(&gb);
+ size_t new_extradata_size = 0;
+ int ret;
- for (i = 0; i < num_arrays; i++) {
- int type = bytestream2_get_byte(&gb) & 0x3f;
- int cnt = bytestream2_get_be16(&gb);
+ for (int i = 0; i < num_arrays; i++) {
+ int type = bytestream2_get_byte(gb) & 0x3f;
+ int cnt = bytestream2_get_be16(gb);
if (!(type == HEVC_NAL_VPS || type == HEVC_NAL_SPS || type == HEVC_NAL_PPS ||
type == HEVC_NAL_SEI_PREFIX || type == HEVC_NAL_SEI_SUFFIX)) {
- av_log(ctx, AV_LOG_ERROR, "Invalid NAL unit type in extradata: %d\n",
+ av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit type in extradata: %d\n",
type);
ret = AVERROR_INVALIDDATA;
goto fail;
}
- for (j = 0; j < cnt; j++) {
- const int nalu_len = bytestream2_get_be16(&gb);
+ for (int j = 0; j < cnt; j++) {
+ const int nalu_len = bytestream2_get_be16(gb);
if (!nalu_len ||
- nalu_len > bytestream2_get_bytes_left(&gb) ||
+ nalu_len > bytestream2_get_bytes_left(gb) ||
4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
ret = AVERROR_INVALIDDATA;
goto fail;
@@ -78,11 +72,38 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
goto fail;
AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
- bytestream2_get_buffer(&gb, new_extradata + new_extradata_size + 4, nalu_len);
+ bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4, nalu_len);
new_extradata_size += 4 + nalu_len;
memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
}
}
+ *new_extradatap = new_extradata;
+ *new_extradata_sizep = new_extradata_size;
+
+ return 0;
+fail:
+ av_freep(&new_extradata);
+ return ret;
+}
+
+static int hevc_extradata_to_annexb(AVBSFContext *ctx)
+{
+ GetByteContext gb;
+ int length_size;
+ int ret = 0;
+
+ uint8_t *new_extradata = NULL;
+ size_t new_extradata_size;
+
+ bytestream2_init(&gb, ctx->par_in->extradata, ctx->par_in->extradata_size);
+
+ bytestream2_skip(&gb, 21);
+ length_size = (bytestream2_get_byte(&gb) & 3) + 1;
+
+ ret = hevc_extradata_to_annexb_internal(ctx, &gb, &new_extradata,
+ &new_extradata_size);
+ if (ret < 0)
+ return ret;
av_freep(&ctx->par_out->extradata);
ctx->par_out->extradata = new_extradata;
@@ -92,9 +113,6 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
av_log(ctx, AV_LOG_WARNING, "No parameter sets in the extradata\n");
return length_size;
-fail:
- av_freep(&new_extradata);
- return ret;
}
static int hevc_mp4toannexb_init(AVBSFContext *ctx)
--
2.34.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* [FFmpeg-devel] [PATCH 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata
2024-02-18 2:41 [FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX Andreas Rheinhardt
2024-02-18 2:44 ` [FFmpeg-devel] [PATCH 2/5] avcodec/bsf/hevc_mp4toannexb: Factor creating new extradata out Andreas Rheinhardt
@ 2024-02-18 2:44 ` Andreas Rheinhardt
2024-02-19 2:07 ` James Almer
2024-02-18 2:44 ` [FFmpeg-devel] [PATCH 4/5] avcodec/bsf/vvc_mp4toannexb: Factor creating new extradata out Andreas Rheinhardt
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Andreas Rheinhardt @ 2024-02-18 2:44 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Andreas Rheinhardt
AVCodecParameters.extradata is supposed to be allocated with
av_malloc(); av_realloc() and its wrappers do not guarantee
the proper alignment. Therefore parse the extradata twice:
Once to check its validity and to determine the eventual size
and a second time to actually write the new extradata.
(Of course, not reallocating the buffer is beneficial in itself.)
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
libavcodec/bsf/hevc_mp4toannexb.c | 44 +++++++++++++++----------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c
index a695cba370..f5424e95b8 100644
--- a/libavcodec/bsf/hevc_mp4toannexb.c
+++ b/libavcodec/bsf/hevc_mp4toannexb.c
@@ -38,13 +38,11 @@ typedef struct HEVCBSFContext {
} HEVCBSFContext;
static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
- uint8_t **new_extradatap,
+ uint8_t *new_extradata,
size_t *new_extradata_sizep)
{
int num_arrays = bytestream2_get_byte(gb);
- uint8_t *new_extradata = NULL;
size_t new_extradata_size = 0;
- int ret;
for (int i = 0; i < num_arrays; i++) {
int type = bytestream2_get_byte(gb) & 0x3f;
@@ -54,8 +52,7 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
type == HEVC_NAL_SEI_PREFIX || type == HEVC_NAL_SEI_SUFFIX)) {
av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit type in extradata: %d\n",
type);
- ret = AVERROR_INVALIDDATA;
- goto fail;
+ return AVERROR_INVALIDDATA;
}
for (int j = 0; j < cnt; j++) {
@@ -64,26 +61,19 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
if (!nalu_len ||
nalu_len > bytestream2_get_bytes_left(gb) ||
4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
- ret = AVERROR_INVALIDDATA;
- goto fail;
+ return AVERROR_INVALIDDATA;
}
- ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4 + AV_INPUT_BUFFER_PADDING_SIZE);
- if (ret < 0)
- goto fail;
-
- AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
- bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4, nalu_len);
+ if (new_extradata) {
+ AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
+ bytestream2_get_bufferu(gb, new_extradata + new_extradata_size + 4, nalu_len);
+ } else
+ bytestream2_skipu(gb, nalu_len);
new_extradata_size += 4 + nalu_len;
- memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
}
}
- *new_extradatap = new_extradata;
*new_extradata_sizep = new_extradata_size;
return 0;
-fail:
- av_freep(&new_extradata);
- return ret;
}
static int hevc_extradata_to_annexb(AVBSFContext *ctx)
@@ -100,10 +90,20 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
bytestream2_skip(&gb, 21);
length_size = (bytestream2_get_byte(&gb) & 3) + 1;
- ret = hevc_extradata_to_annexb_internal(ctx, &gb, &new_extradata,
- &new_extradata_size);
- if (ret < 0)
- return ret;
+ while (1) {
+ GetByteContext gb_bak = gb;
+ ret = hevc_extradata_to_annexb_internal(ctx, &gb, new_extradata,
+ &new_extradata_size);
+ if (ret < 0)
+ return ret;
+ if (new_extradata || !new_extradata_size)
+ break;
+ new_extradata = av_malloc(new_extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
+ if (!new_extradata)
+ return AVERROR(ENOMEM);
+ memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
+ gb = gb_bak;
+ }
av_freep(&ctx->par_out->extradata);
ctx->par_out->extradata = new_extradata;
--
2.34.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [FFmpeg-devel] [PATCH 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata
2024-02-18 2:44 ` [FFmpeg-devel] [PATCH 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata Andreas Rheinhardt
@ 2024-02-19 2:07 ` James Almer
2024-02-19 10:56 ` Andreas Rheinhardt
0 siblings, 1 reply; 9+ messages in thread
From: James Almer @ 2024-02-19 2:07 UTC (permalink / raw)
To: ffmpeg-devel
On 2/17/2024 11:44 PM, Andreas Rheinhardt wrote:
> AVCodecParameters.extradata is supposed to be allocated with
> av_malloc(); av_realloc() and its wrappers do not guarantee
> the proper alignment. Therefore parse the extradata twice:
> Once to check its validity and to determine the eventual size
> and a second time to actually write the new extradata.
Why would av_malloc alignment be needed for extradata?
I see the doxy says "Must be allocated with av_malloc()" but I'm fairly
sure that was meant to be "Must be allocated with av_malloc() family of
functions", like its AVCodecContext counterpart. The idea is that
library users don't use normal malloc as extradata will be freed with
av_free(), and not because it will be accessed by SIMD code.
>
> (Of course, not reallocating the buffer is beneficial in itself.)
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> libavcodec/bsf/hevc_mp4toannexb.c | 44 +++++++++++++++----------------
> 1 file changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c
> index a695cba370..f5424e95b8 100644
> --- a/libavcodec/bsf/hevc_mp4toannexb.c
> +++ b/libavcodec/bsf/hevc_mp4toannexb.c
> @@ -38,13 +38,11 @@ typedef struct HEVCBSFContext {
> } HEVCBSFContext;
>
> static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
> - uint8_t **new_extradatap,
> + uint8_t *new_extradata,
> size_t *new_extradata_sizep)
> {
> int num_arrays = bytestream2_get_byte(gb);
> - uint8_t *new_extradata = NULL;
> size_t new_extradata_size = 0;
> - int ret;
>
> for (int i = 0; i < num_arrays; i++) {
> int type = bytestream2_get_byte(gb) & 0x3f;
> @@ -54,8 +52,7 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
> type == HEVC_NAL_SEI_PREFIX || type == HEVC_NAL_SEI_SUFFIX)) {
> av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit type in extradata: %d\n",
> type);
> - ret = AVERROR_INVALIDDATA;
> - goto fail;
> + return AVERROR_INVALIDDATA;
> }
>
> for (int j = 0; j < cnt; j++) {
> @@ -64,26 +61,19 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
> if (!nalu_len ||
> nalu_len > bytestream2_get_bytes_left(gb) ||
> 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
> - ret = AVERROR_INVALIDDATA;
> - goto fail;
> + return AVERROR_INVALIDDATA;
> }
> - ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4 + AV_INPUT_BUFFER_PADDING_SIZE);
> - if (ret < 0)
> - goto fail;
> -
> - AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
> - bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4, nalu_len);
> + if (new_extradata) {
> + AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
> + bytestream2_get_bufferu(gb, new_extradata + new_extradata_size + 4, nalu_len);
> + } else
> + bytestream2_skipu(gb, nalu_len);
> new_extradata_size += 4 + nalu_len;
> - memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> }
> }
> - *new_extradatap = new_extradata;
> *new_extradata_sizep = new_extradata_size;
>
> return 0;
> -fail:
> - av_freep(&new_extradata);
> - return ret;
> }
>
> static int hevc_extradata_to_annexb(AVBSFContext *ctx)
> @@ -100,10 +90,20 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
> bytestream2_skip(&gb, 21);
> length_size = (bytestream2_get_byte(&gb) & 3) + 1;
>
> - ret = hevc_extradata_to_annexb_internal(ctx, &gb, &new_extradata,
> - &new_extradata_size);
> - if (ret < 0)
> - return ret;
> + while (1) {
> + GetByteContext gb_bak = gb;
> + ret = hevc_extradata_to_annexb_internal(ctx, &gb, new_extradata,
> + &new_extradata_size);
> + if (ret < 0)
> + return ret;
> + if (new_extradata || !new_extradata_size)
> + break;
> + new_extradata = av_malloc(new_extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
> + if (!new_extradata)
> + return AVERROR(ENOMEM);
> + memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> + gb = gb_bak;
> + }
>
> av_freep(&ctx->par_out->extradata);
> ctx->par_out->extradata = new_extradata;
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [FFmpeg-devel] [PATCH 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata
2024-02-19 2:07 ` James Almer
@ 2024-02-19 10:56 ` Andreas Rheinhardt
0 siblings, 0 replies; 9+ messages in thread
From: Andreas Rheinhardt @ 2024-02-19 10:56 UTC (permalink / raw)
To: ffmpeg-devel
James Almer:
> On 2/17/2024 11:44 PM, Andreas Rheinhardt wrote:
>> AVCodecParameters.extradata is supposed to be allocated with
>> av_malloc(); av_realloc() and its wrappers do not guarantee
>> the proper alignment. Therefore parse the extradata twice:
>> Once to check its validity and to determine the eventual size
>> and a second time to actually write the new extradata.
>
> Why would av_malloc alignment be needed for extradata?
> I see the doxy says "Must be allocated with av_malloc()" but I'm fairly
> sure that was meant to be "Must be allocated with av_malloc() family of
> functions", like its AVCodecContext counterpart. The idea is that
> library users don't use normal malloc as extradata will be freed with
> av_free(), and not because it will be accessed by SIMD code.
>
I thought that potential accesses by SIMD code was the point? After all,
the value of AV_INPUT_BUFFER_PADDING_SIZE (which is used for both packet
data and extradata) is chosen so big to accommodate reading via SIMD.
You incremented the constant for this very purpose in 6e80079a28.
(Granted, I don't think we have code where extradata is being parsed by
SIMD.)
Apart from that, given its differing alignment, I am not sure that
av_realloc() is really part of the av_malloc() family of functions. We
should probably replace "av_malloc() family" by "compatible with
av_free()" wherever we want to allow av_realloc(), too.
Anyway, there is another advantage of this patch:
>>
>> (Of course, not reallocating the buffer is beneficial in itself.)
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread
* [FFmpeg-devel] [PATCH 4/5] avcodec/bsf/vvc_mp4toannexb: Factor creating new extradata out
2024-02-18 2:41 [FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX Andreas Rheinhardt
2024-02-18 2:44 ` [FFmpeg-devel] [PATCH 2/5] avcodec/bsf/hevc_mp4toannexb: Factor creating new extradata out Andreas Rheinhardt
2024-02-18 2:44 ` [FFmpeg-devel] [PATCH 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata Andreas Rheinhardt
@ 2024-02-18 2:44 ` Andreas Rheinhardt
2024-02-18 2:44 ` [FFmpeg-devel] [PATCH 5/5] avcodec/bsf/vvc_mp4toannexb: Don't realloc when creating new extradata Andreas Rheinhardt
2024-02-18 2:50 ` [FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX James Almer
4 siblings, 0 replies; 9+ messages in thread
From: Andreas Rheinhardt @ 2024-02-18 2:44 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Andreas Rheinhardt
This is in preparation for the next commit.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
libavcodec/bsf/vvc_mp4toannexb.c | 116 ++++++++++++++++++-------------
1 file changed, 67 insertions(+), 49 deletions(-)
diff --git a/libavcodec/bsf/vvc_mp4toannexb.c b/libavcodec/bsf/vvc_mp4toannexb.c
index 1b851f3223..bfb0338116 100644
--- a/libavcodec/bsf/vvc_mp4toannexb.c
+++ b/libavcodec/bsf/vvc_mp4toannexb.c
@@ -37,16 +37,77 @@ typedef struct VVCBSFContext {
int extradata_parsed;
} VVCBSFContext;
+static int vvc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
+ uint8_t **new_extradatap,
+ size_t *new_extradata_sizep)
+{
+ int num_arrays = bytestream2_get_byte(gb);
+ uint8_t *new_extradata = NULL;
+ size_t new_extradata_size = 0;
+ int ret;
+
+ for (int i = 0; i < num_arrays; i++) {
+ int cnt;
+ int type = bytestream2_get_byte(gb) & 0x1f;
+
+ if (type == VVC_OPI_NUT || type == VVC_DCI_NUT)
+ cnt = 1;
+ else
+ cnt = bytestream2_get_be16(gb);
+
+ av_log(logctx, AV_LOG_DEBUG, "nalu_type %d cnt %d\n", type, cnt);
+
+ if (!(type == VVC_OPI_NUT || type == VVC_DCI_NUT ||
+ type == VVC_VPS_NUT || type == VVC_SPS_NUT || type == VVC_PPS_NUT
+ || type == VVC_PREFIX_SEI_NUT || type == VVC_SUFFIX_SEI_NUT)) {
+ av_log(logctx, AV_LOG_ERROR,
+ "Invalid NAL unit type in extradata: %d\n", type);
+ ret = AVERROR_INVALIDDATA;
+ goto fail;
+ }
+
+ for (int j = 0; j < cnt; j++) {
+ const int nalu_len = bytestream2_get_be16(gb);
+
+ if (!nalu_len ||
+ nalu_len > bytestream2_get_bytes_left(gb) ||
+ 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
+ ret = AVERROR_INVALIDDATA;
+ goto fail;
+ }
+ ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4
+ + AV_INPUT_BUFFER_PADDING_SIZE);
+ if (ret < 0)
+ goto fail;
+
+ AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
+ bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4,
+ nalu_len);
+ new_extradata_size += 4 + nalu_len;
+ memset(new_extradata + new_extradata_size, 0,
+ AV_INPUT_BUFFER_PADDING_SIZE);
+ }
+ }
+
+ *new_extradatap = new_extradata;
+ *new_extradata_sizep = new_extradata_size;
+
+ return 0;
+fail:
+ av_freep(&new_extradata);
+ return ret;
+}
+
static int vvc_extradata_to_annexb(AVBSFContext *ctx)
{
GetByteContext gb;
- int length_size, num_arrays, i, j;
+ int length_size, i, j;
int ret = 0;
int temp = 0;
int ptl_present;
uint8_t *new_extradata = NULL;
- size_t new_extradata_size = 0;
+ size_t new_extradata_size;
int max_picture_width = 0;
int max_picture_height = 0;
@@ -132,50 +193,10 @@ static int vvc_extradata_to_annexb(AVBSFContext *ctx)
max_picture_width, max_picture_height, avg_frame_rate);
}
- num_arrays = bytestream2_get_byte(&gb);
-
- for (i = 0; i < num_arrays; i++) {
- int cnt;
- int type = bytestream2_get_byte(&gb) & 0x1f;
-
- if (type == VVC_OPI_NUT || type == VVC_DCI_NUT)
- cnt = 1;
- else
- cnt = bytestream2_get_be16(&gb);
-
- av_log(ctx, AV_LOG_DEBUG, "nalu_type %d cnt %d\n", type, cnt);
-
- if (!(type == VVC_OPI_NUT || type == VVC_DCI_NUT ||
- type == VVC_VPS_NUT || type == VVC_SPS_NUT || type == VVC_PPS_NUT
- || type == VVC_PREFIX_SEI_NUT || type == VVC_SUFFIX_SEI_NUT)) {
- av_log(ctx, AV_LOG_ERROR,
- "Invalid NAL unit type in extradata: %d\n", type);
- ret = AVERROR_INVALIDDATA;
- goto fail;
- }
-
- for (j = 0; j < cnt; j++) {
- const int nalu_len = bytestream2_get_be16(&gb);
-
- if (!nalu_len ||
- nalu_len > bytestream2_get_bytes_left(&gb) ||
- 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
- ret = AVERROR_INVALIDDATA;
- goto fail;
- }
- ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4
- + AV_INPUT_BUFFER_PADDING_SIZE);
- if (ret < 0)
- goto fail;
-
- AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
- bytestream2_get_buffer(&gb, new_extradata + new_extradata_size + 4,
- nalu_len);
- new_extradata_size += 4 + nalu_len;
- memset(new_extradata + new_extradata_size, 0,
- AV_INPUT_BUFFER_PADDING_SIZE);
- }
- }
+ ret = vvc_extradata_to_annexb_internal(ctx, &gb, &new_extradata,
+ &new_extradata_size);
+ if (ret < 0)
+ return ret;
av_freep(&ctx->par_out->extradata);
ctx->par_out->extradata = new_extradata;
@@ -185,9 +206,6 @@ static int vvc_extradata_to_annexb(AVBSFContext *ctx)
av_log(ctx, AV_LOG_WARNING, "No parameter sets in the extradata\n");
return length_size;
- fail:
- av_freep(&new_extradata);
- return ret;
}
static int vvc_mp4toannexb_init(AVBSFContext *ctx)
--
2.34.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* [FFmpeg-devel] [PATCH 5/5] avcodec/bsf/vvc_mp4toannexb: Don't realloc when creating new extradata
2024-02-18 2:41 [FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX Andreas Rheinhardt
` (2 preceding siblings ...)
2024-02-18 2:44 ` [FFmpeg-devel] [PATCH 4/5] avcodec/bsf/vvc_mp4toannexb: Factor creating new extradata out Andreas Rheinhardt
@ 2024-02-18 2:44 ` Andreas Rheinhardt
2024-02-18 2:50 ` [FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX James Almer
4 siblings, 0 replies; 9+ messages in thread
From: Andreas Rheinhardt @ 2024-02-18 2:44 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Andreas Rheinhardt
AVCodecParameters.extradata is supposed to be allocated with
av_malloc(); av_realloc() and its wrappers do not guarantee
the proper alignment. Therefore parse the extradata twice:
Once to check its validity and to determine the eventual size
and a second time to actually write the new extradata.
(Of course, not reallocating the buffer is beneficial in itself.)
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
libavcodec/bsf/vvc_mp4toannexb.c | 51 ++++++++++++++++----------------
1 file changed, 25 insertions(+), 26 deletions(-)
diff --git a/libavcodec/bsf/vvc_mp4toannexb.c b/libavcodec/bsf/vvc_mp4toannexb.c
index bfb0338116..1879c1fab9 100644
--- a/libavcodec/bsf/vvc_mp4toannexb.c
+++ b/libavcodec/bsf/vvc_mp4toannexb.c
@@ -38,13 +38,11 @@ typedef struct VVCBSFContext {
} VVCBSFContext;
static int vvc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
- uint8_t **new_extradatap,
+ uint8_t *new_extradata,
size_t *new_extradata_sizep)
{
int num_arrays = bytestream2_get_byte(gb);
- uint8_t *new_extradata = NULL;
size_t new_extradata_size = 0;
- int ret;
for (int i = 0; i < num_arrays; i++) {
int cnt;
@@ -55,15 +53,15 @@ static int vvc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
else
cnt = bytestream2_get_be16(gb);
- av_log(logctx, AV_LOG_DEBUG, "nalu_type %d cnt %d\n", type, cnt);
+ if (!new_extradata)
+ av_log(logctx, AV_LOG_DEBUG, "nalu_type %d cnt %d\n", type, cnt);
if (!(type == VVC_OPI_NUT || type == VVC_DCI_NUT ||
type == VVC_VPS_NUT || type == VVC_SPS_NUT || type == VVC_PPS_NUT
|| type == VVC_PREFIX_SEI_NUT || type == VVC_SUFFIX_SEI_NUT)) {
av_log(logctx, AV_LOG_ERROR,
"Invalid NAL unit type in extradata: %d\n", type);
- ret = AVERROR_INVALIDDATA;
- goto fail;
+ return AVERROR_INVALIDDATA;
}
for (int j = 0; j < cnt; j++) {
@@ -72,30 +70,21 @@ static int vvc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
if (!nalu_len ||
nalu_len > bytestream2_get_bytes_left(gb) ||
4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
- ret = AVERROR_INVALIDDATA;
- goto fail;
+ return AVERROR_INVALIDDATA;
}
- ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4
- + AV_INPUT_BUFFER_PADDING_SIZE);
- if (ret < 0)
- goto fail;
-
- AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
- bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4,
- nalu_len);
+ if (new_extradata) {
+ AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
+ bytestream2_get_bufferu(gb, new_extradata + new_extradata_size + 4,
+ nalu_len);
+ } else
+ bytestream2_skipu(gb, nalu_len);
new_extradata_size += 4 + nalu_len;
- memset(new_extradata + new_extradata_size, 0,
- AV_INPUT_BUFFER_PADDING_SIZE);
}
}
- *new_extradatap = new_extradata;
*new_extradata_sizep = new_extradata_size;
return 0;
-fail:
- av_freep(&new_extradata);
- return ret;
}
static int vvc_extradata_to_annexb(AVBSFContext *ctx)
@@ -193,10 +182,20 @@ static int vvc_extradata_to_annexb(AVBSFContext *ctx)
max_picture_width, max_picture_height, avg_frame_rate);
}
- ret = vvc_extradata_to_annexb_internal(ctx, &gb, &new_extradata,
- &new_extradata_size);
- if (ret < 0)
- return ret;
+ while (1) {
+ GetByteContext gb_bak = gb;
+ ret = vvc_extradata_to_annexb_internal(ctx, &gb, new_extradata,
+ &new_extradata_size);
+ if (ret < 0)
+ return ret;
+ if (new_extradata || !new_extradata_size)
+ break;
+ new_extradata = av_malloc(new_extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
+ if (!new_extradata)
+ return AVERROR(ENOMEM);
+ memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
+ gb = gb_bak;
+ }
av_freep(&ctx->par_out->extradata);
ctx->par_out->extradata = new_extradata;
--
2.34.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX
2024-02-18 2:41 [FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX Andreas Rheinhardt
` (3 preceding siblings ...)
2024-02-18 2:44 ` [FFmpeg-devel] [PATCH 5/5] avcodec/bsf/vvc_mp4toannexb: Don't realloc when creating new extradata Andreas Rheinhardt
@ 2024-02-18 2:50 ` James Almer
2024-02-18 12:16 ` Andreas Rheinhardt
4 siblings, 1 reply; 9+ messages in thread
From: James Almer @ 2024-02-18 2:50 UTC (permalink / raw)
To: ffmpeg-devel
On 2/17/2024 11:41 PM, Andreas Rheinhardt wrote:
> AVCodecParameters.extradata_size is an int.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> libavcodec/bsf/hevc_mp4toannexb.c | 2 +-
> libavcodec/bsf/vvc_mp4toannexb.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c
> index 8eec18f31e..c0df2b79a6 100644
> --- a/libavcodec/bsf/hevc_mp4toannexb.c
> +++ b/libavcodec/bsf/hevc_mp4toannexb.c
> @@ -69,7 +69,7 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
>
> if (!nalu_len ||
> nalu_len > bytestream2_get_bytes_left(&gb) ||
> - 4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > SIZE_MAX - new_extradata_size) {
> + 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
> ret = AVERROR_INVALIDDATA;
> goto fail;
> }
> diff --git a/libavcodec/bsf/vvc_mp4toannexb.c b/libavcodec/bsf/vvc_mp4toannexb.c
> index 36bdae8f49..1b851f3223 100644
> --- a/libavcodec/bsf/vvc_mp4toannexb.c
> +++ b/libavcodec/bsf/vvc_mp4toannexb.c
> @@ -159,7 +159,7 @@ static int vvc_extradata_to_annexb(AVBSFContext *ctx)
>
> if (!nalu_len ||
> nalu_len > bytestream2_get_bytes_left(&gb) ||
> - 4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > SIZE_MAX - new_extradata_size) {
> + 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
Just use INT_MAX, there's no point in this check. Do you expect a system
where an int is smaller than the type meant to store size of buffers in
memory?
> ret = AVERROR_INVALIDDATA;
> goto fail;
> }
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX
2024-02-18 2:50 ` [FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX James Almer
@ 2024-02-18 12:16 ` Andreas Rheinhardt
0 siblings, 0 replies; 9+ messages in thread
From: Andreas Rheinhardt @ 2024-02-18 12:16 UTC (permalink / raw)
To: ffmpeg-devel
James Almer:
> On 2/17/2024 11:41 PM, Andreas Rheinhardt wrote:
>> AVCodecParameters.extradata_size is an int.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> libavcodec/bsf/hevc_mp4toannexb.c | 2 +-
>> libavcodec/bsf/vvc_mp4toannexb.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/bsf/hevc_mp4toannexb.c
>> b/libavcodec/bsf/hevc_mp4toannexb.c
>> index 8eec18f31e..c0df2b79a6 100644
>> --- a/libavcodec/bsf/hevc_mp4toannexb.c
>> +++ b/libavcodec/bsf/hevc_mp4toannexb.c
>> @@ -69,7 +69,7 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
>> if (!nalu_len ||
>> nalu_len > bytestream2_get_bytes_left(&gb) ||
>> - 4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len >
>> SIZE_MAX - new_extradata_size) {
>> + 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) -
>> AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
>> ret = AVERROR_INVALIDDATA;
>> goto fail;
>> }
>> diff --git a/libavcodec/bsf/vvc_mp4toannexb.c
>> b/libavcodec/bsf/vvc_mp4toannexb.c
>> index 36bdae8f49..1b851f3223 100644
>> --- a/libavcodec/bsf/vvc_mp4toannexb.c
>> +++ b/libavcodec/bsf/vvc_mp4toannexb.c
>> @@ -159,7 +159,7 @@ static int vvc_extradata_to_annexb(AVBSFContext *ctx)
>> if (!nalu_len ||
>> nalu_len > bytestream2_get_bytes_left(&gb) ||
>> - 4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len >
>> SIZE_MAX - new_extradata_size) {
>> + 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) -
>> AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
>
> Just use INT_MAX, there's no point in this check. Do you expect a system
> where an int is smaller than the type meant to store size of buffers in
> memory?
>
We typically try to avoid such assumptions (although we actually have
lots of INT_MAX <= SIZE_MAX assumptions in the codebase, namely lots of
instances where one uses an int and an allocation function).
In this case using FFMIN(INT_MAX, SIZE_MAX) will make it easier to find
these lines via git grep when extradata_size is switched to size_t.
Alternatively, one could add an AV_CODECPAR_MAX_EXTRADATA_SIZE define,
but this is even longer than FFMIN(INT_MAX, SIZE_MAX).
- Andreas
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread