Re: [FFmpeg-devel] [PATCH 2/3] libavcodec/cbs: Don't zero twice
On 10/02/2019 23:11, Andreas Rheinhardt wrote:
> Mark Thompson:
>> On 05/02/2019 20:08, Andreas Rheinhardt wrote:
>>> int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>>>CodedBitstreamFragment *frag,
>>> - const AVCodecParameters *par)
>>> + const AVCodecParameters *par,
>>> + int reuse)
>>> {
>>> int err;
>>>
>>> -memset(frag, 0, sizeof(*frag));
>>> +if (!reuse)
>>> +memset(frag, 0, sizeof(*frag));
>>>
>>> err = cbs_fill_fragment_data(ctx, frag, par->extradata,
>>> par->extradata_size);
>>> @@ -237,11 +239,12 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>>>
>>> int ff_cbs_read_packet(CodedBitstreamContext *ctx,
>>> CodedBitstreamFragment *frag,
>>> - const AVPacket *pkt)
>>> + const AVPacket *pkt, int reuse)
>>> {
>>> int err;
>>>
>>> -memset(frag, 0, sizeof(*frag));
>>> +if (!reuse)
>>> +memset(frag, 0, sizeof(*frag));
>>>
>>> if (pkt->buf) {
>>> frag->data_ref = av_buffer_ref(pkt->buf);
>>> @@ -266,11 +269,13 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx,
>>>
>>> int ff_cbs_read(CodedBitstreamContext *ctx,
>>> CodedBitstreamFragment *frag,
>>> -const uint8_t *data, size_t size)
>>> +const uint8_t *data, size_t size,
>>> +int reuse)
>>> {
>>> int err;
>>>
>>> -memset(frag, 0, sizeof(*frag));
>>> +if (!reuse)
>>> +memset(frag, 0, sizeof(*frag));
>>>
>>> err = cbs_fill_fragment_data(ctx, frag, data, size);
>>> if (err < 0)
>>
>> I don't think this patch should be needed. Can we just document that your
>> fragment must either be zeroed (so, allocated by av_*allocz() or memset() to
>> zero) or you've called the ff_cbs_fragment_reset() (whatever the name is)
>> function before any read call? It can even check that the user hasn't
>> messed up by asserting that data, data_size and nb_units are all zero.
>
> I agree with your suggestion regarding the documentation; but it is
> nevertheless needed to eliminate the memset in the ff_cbs_read
> functions. Otherwise the unit array is leaked.
Yes, sorry. Indeed still remove the memset, but don't change any of the
parameters or callers.
Thanks,
- Mark
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/3] libavcodec/cbs: Don't zero twice
Mark Thompson:
> On 05/02/2019 20:08, Andreas Rheinhardt wrote:
>> int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>>CodedBitstreamFragment *frag,
>> - const AVCodecParameters *par)
>> + const AVCodecParameters *par,
>> + int reuse)
>> {
>> int err;
>>
>> -memset(frag, 0, sizeof(*frag));
>> +if (!reuse)
>> +memset(frag, 0, sizeof(*frag));
>>
>> err = cbs_fill_fragment_data(ctx, frag, par->extradata,
>> par->extradata_size);
>> @@ -237,11 +239,12 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>>
>> int ff_cbs_read_packet(CodedBitstreamContext *ctx,
>> CodedBitstreamFragment *frag,
>> - const AVPacket *pkt)
>> + const AVPacket *pkt, int reuse)
>> {
>> int err;
>>
>> -memset(frag, 0, sizeof(*frag));
>> +if (!reuse)
>> +memset(frag, 0, sizeof(*frag));
>>
>> if (pkt->buf) {
>> frag->data_ref = av_buffer_ref(pkt->buf);
>> @@ -266,11 +269,13 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx,
>>
>> int ff_cbs_read(CodedBitstreamContext *ctx,
>> CodedBitstreamFragment *frag,
>> -const uint8_t *data, size_t size)
>> +const uint8_t *data, size_t size,
>> +int reuse)
>> {
>> int err;
>>
>> -memset(frag, 0, sizeof(*frag));
>> +if (!reuse)
>> +memset(frag, 0, sizeof(*frag));
>>
>> err = cbs_fill_fragment_data(ctx, frag, data, size);
>> if (err < 0)
>
> I don't think this patch should be needed. Can we just document that your
> fragment must either be zeroed (so, allocated by av_*allocz() or memset() to
> zero) or you've called the ff_cbs_fragment_reset() (whatever the name is)
> function before any read call? It can even check that the user hasn't messed
> up by asserting that data, data_size and nb_units are all zero.
I agree with your suggestion regarding the documentation; but it is
nevertheless needed to eliminate the memset in the ff_cbs_read
functions. Otherwise the unit array is leaked.
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/3] libavcodec/cbs: Don't zero twice
On 05/02/2019 20:08, Andreas Rheinhardt wrote:
> Up until now, a fragment that got reused was zeroed twice: Once during
> uninit and once during reading the next packet/extradata/buffer. The
> second zeroing has now been made optional.
>
> This is also in preparation of actually reusing a fragment's units array.
> Otherwise it would be lost during reading.
>
> Signed-off-by: Andreas Rheinhardt
> ---
> libavcodec/av1_metadata_bsf.c | 4 ++--
> libavcodec/av1_parser.c | 4 ++--
> libavcodec/cbs.c| 17 +++--
> libavcodec/cbs.h| 20 +---
> libavcodec/filter_units_bsf.c | 4 ++--
> libavcodec/h264_metadata_bsf.c | 4 ++--
> libavcodec/h264_redundant_pps_bsf.c | 4 ++--
> libavcodec/h265_metadata_bsf.c | 4 ++--
> libavcodec/mpeg2_metadata_bsf.c | 4 ++--
> libavcodec/trace_headers_bsf.c | 4 ++--
> libavcodec/vp9_metadata_bsf.c | 2 +-
> 11 files changed, 45 insertions(+), 26 deletions(-)
>
> ...
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index b61dedb1eb..71f9fcbe32 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -217,11 +217,13 @@ static int cbs_fill_fragment_data(CodedBitstreamContext
> *ctx,
>
> int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>CodedBitstreamFragment *frag,
> - const AVCodecParameters *par)
> + const AVCodecParameters *par,
> + int reuse)
> {
> int err;
>
> -memset(frag, 0, sizeof(*frag));
> +if (!reuse)
> +memset(frag, 0, sizeof(*frag));
>
> err = cbs_fill_fragment_data(ctx, frag, par->extradata,
> par->extradata_size);
> @@ -237,11 +239,12 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>
> int ff_cbs_read_packet(CodedBitstreamContext *ctx,
> CodedBitstreamFragment *frag,
> - const AVPacket *pkt)
> + const AVPacket *pkt, int reuse)
> {
> int err;
>
> -memset(frag, 0, sizeof(*frag));
> +if (!reuse)
> +memset(frag, 0, sizeof(*frag));
>
> if (pkt->buf) {
> frag->data_ref = av_buffer_ref(pkt->buf);
> @@ -266,11 +269,13 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx,
>
> int ff_cbs_read(CodedBitstreamContext *ctx,
> CodedBitstreamFragment *frag,
> -const uint8_t *data, size_t size)
> +const uint8_t *data, size_t size,
> +int reuse)
> {
> int err;
>
> -memset(frag, 0, sizeof(*frag));
> +if (!reuse)
> +memset(frag, 0, sizeof(*frag));
>
> err = cbs_fill_fragment_data(ctx, frag, data, size);
> if (err < 0)
> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
> index 229cb129aa..2265b5d5bd 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -240,10 +240,15 @@ void ff_cbs_close(CodedBitstreamContext **ctx);
> * This also updates the internal state, so will need to be called for
> * codecs with extradata to read parameter sets necessary for further
> * parsing even if the fragment itself is not desired.
> + *
> + * If reuse is not set, the fragment will be zeroed before usage;
> + * otherwise, the fragment's units_allocated and units members must
> + * be valid and all the other members have to be zero/NULL.
> */
> int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>CodedBitstreamFragment *frag,
> - const AVCodecParameters *par);
> + const AVCodecParameters *par,
> + int reuse);
>
> /**
> * Read the data bitstream from a packet into a fragment, then
> @@ -252,10 +257,14 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
> * This also updates the internal state of the coded bitstream context
> * with any persistent data from the fragment which may be required to
> * read following fragments (e.g. parameter sets).
> + *
> + * If reuse is not set, the fragment will be zeroed before usage;
> + * otherwise, the fragment's units_allocated and units members must
> + * be valid and all the other members have to be zero/NULL.
> */
> int ff_cbs_read_packet(CodedBitstreamContext *ctx,
> CodedBitstreamFragment *frag,
> - const AVPacket *pkt);
> + const AVPacket *pkt, int reuse);
>
> /**
> * Read a bitstream from a memory region into a fragment, then
> @@ -264,10 +273,15 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx,
> * This also updates the internal state of the coded bitstream context
> * with any persistent data from the fragment which may be required to
> * read following fragments (e.g. parameter sets).
> + *
> + * If reuse is not set, the fragment will be zeroed before usage;
> + * otherwise, the fragment's units_allocated and
