| Message ID | 20180424021700.11720-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 4/23/2018 11:17 PM, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/cbs_mpeg2.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c > index bfb64a0851..086d08ed64 100644 > --- a/libavcodec/cbs_mpeg2.c > +++ b/libavcodec/cbs_mpeg2.c > @@ -146,14 +146,17 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, > unit_size = (end - 4) - (start - 1); > } > > - unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE); > - if (!unit_data) > - return AVERROR(ENOMEM); > - memcpy(unit_data, start - 1, unit_size); > - memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); > + if (header) { > + unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE); > + if (!unit_data) > + return AVERROR(ENOMEM); > + memcpy(unit_data, start - 1, unit_size); > + memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); > + } else > + unit_data = (uint8_t *)start - 1; > > err = ff_cbs_insert_unit_data(ctx, frag, i, unit_type, > - unit_data, unit_size, NULL); > + unit_data, unit_size, frag->data_ref); > if (err < 0) { > av_freep(&unit_data); > return err; The alternative is to make ff_cbs_read_extradata() create an AVBufferRef with the contents of par->extradata, so every CodedBitstreamType->split_fragment() function can safely assume the fragment will always be reference counted. I think that would be cleaner overall.
On 4/24/2018 7:22 PM, James Almer wrote: > On 4/23/2018 11:17 PM, James Almer wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/cbs_mpeg2.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c >> index bfb64a0851..086d08ed64 100644 >> --- a/libavcodec/cbs_mpeg2.c >> +++ b/libavcodec/cbs_mpeg2.c >> @@ -146,14 +146,17 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, >> unit_size = (end - 4) - (start - 1); >> } >> >> - unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE); >> - if (!unit_data) >> - return AVERROR(ENOMEM); >> - memcpy(unit_data, start - 1, unit_size); >> - memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); >> + if (header) { >> + unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE); >> + if (!unit_data) >> + return AVERROR(ENOMEM); >> + memcpy(unit_data, start - 1, unit_size); >> + memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); >> + } else >> + unit_data = (uint8_t *)start - 1; >> >> err = ff_cbs_insert_unit_data(ctx, frag, i, unit_type, >> - unit_data, unit_size, NULL); >> + unit_data, unit_size, frag->data_ref); >> if (err < 0) { >> av_freep(&unit_data); And this is obviously going to crash if header == 0... I'll wait until the suggestion below is accepted or not before sending a fixed version. >> return err; > > The alternative is to make ff_cbs_read_extradata() create an AVBufferRef > with the contents of par->extradata, so every > CodedBitstreamType->split_fragment() function can safely assume the > fragment will always be reference counted. > > I think that would be cleaner overall. >
On 24/04/18 23:22, James Almer wrote: > On 4/23/2018 11:17 PM, James Almer wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/cbs_mpeg2.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c >> index bfb64a0851..086d08ed64 100644 >> --- a/libavcodec/cbs_mpeg2.c >> +++ b/libavcodec/cbs_mpeg2.c >> @@ -146,14 +146,17 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, >> unit_size = (end - 4) - (start - 1); >> } >> >> - unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE); >> - if (!unit_data) >> - return AVERROR(ENOMEM); >> - memcpy(unit_data, start - 1, unit_size); >> - memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); >> + if (header) { >> + unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE); >> + if (!unit_data) >> + return AVERROR(ENOMEM); >> + memcpy(unit_data, start - 1, unit_size); >> + memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); >> + } else >> + unit_data = (uint8_t *)start - 1; >> >> err = ff_cbs_insert_unit_data(ctx, frag, i, unit_type, >> - unit_data, unit_size, NULL); >> + unit_data, unit_size, frag->data_ref); >> if (err < 0) { >> av_freep(&unit_data); >> return err; > > The alternative is to make ff_cbs_read_extradata() create an AVBufferRef > with the contents of par->extradata, so every > CodedBitstreamType->split_fragment() function can safely assume the > fragment will always be reference counted. > > I think that would be cleaner overall. I agree, it would be much cleaner if we can assume that the input fragment is always refcounted. (The AV1 code here assumes it already too, though we didn't have extradata for it at the time.) - Mark
diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c index bfb64a0851..086d08ed64 100644 --- a/libavcodec/cbs_mpeg2.c +++ b/libavcodec/cbs_mpeg2.c @@ -146,14 +146,17 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, unit_size = (end - 4) - (start - 1); } - unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE); - if (!unit_data) - return AVERROR(ENOMEM); - memcpy(unit_data, start - 1, unit_size); - memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); + if (header) { + unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE); + if (!unit_data) + return AVERROR(ENOMEM); + memcpy(unit_data, start - 1, unit_size); + memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE); + } else + unit_data = (uint8_t *)start - 1; err = ff_cbs_insert_unit_data(ctx, frag, i, unit_type, - unit_data, unit_size, NULL); + unit_data, unit_size, frag->data_ref); if (err < 0) { av_freep(&unit_data); return err;
Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/cbs_mpeg2.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)