Re: [FFmpeg-devel] [PATCH 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata

2024-02-19 Thread Andreas Rheinhardt 
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 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata

2024-02-18 Thread 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.




(Of course, not reallocating the buffer is beneficial in itself.)

Signed-off-by: Andreas Rheinhardt 
---
  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-requ...@ffmpeg.org with subject "unsubs

[FFmpeg-devel] [PATCH 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata

2024-02-17 Thread 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 
---
 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-requ...@ffmpeg.org with subject "unsubscribe".