On 09/05/18 00:12, James Almer wrote: > Removes unnecessary data copies, and fixes potential issues with > dangling references held in said lists. > > Signed-off-by: James Almer <jamr...@gmail.com> > --- > libavcodec/cbs_h264.h | 2 ++ > libavcodec/cbs_h2645.c | 41 +++++++++++++++++++++-------------------- > libavcodec/cbs_h265.h | 3 +++ > 3 files changed, 26 insertions(+), 20 deletions(-) > > diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h > index 2219d9da8d..d953c1f66b 100644 > --- a/libavcodec/cbs_h264.h > +++ b/libavcodec/cbs_h264.h > @@ -421,6 +421,8 @@ typedef struct CodedBitstreamH264Context { > > // All currently available parameter sets. These are updated when > // any parameter set NAL unit is read/written with this context. > + AVBufferRef *sps_ref[H264_MAX_SPS_COUNT]; > + AVBufferRef *pps_ref[H264_MAX_PPS_COUNT]; > H264RawSPS *sps[H264_MAX_SPS_COUNT]; > H264RawPPS *pps[H264_MAX_PPS_COUNT]; > > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c > index 64a1a2d1ee..c391988b58 100644 > --- a/libavcodec/cbs_h2645.c > +++ b/libavcodec/cbs_h2645.c > @@ -677,9 +677,10 @@ static int > cbs_h2645_split_fragment(CodedBitstreamContext *ctx, > > #define cbs_h2645_replace_ps(h26n, ps_name, ps_var, id_element) \ > static int cbs_h26 ## h26n ## _replace_ ## ps_var(CodedBitstreamContext > *ctx, \ > - const H26 ## h26n ## Raw > ## ps_name *ps_var) \ > + CodedBitstreamUnit *unit) > \ > { \ > CodedBitstreamH26 ## h26n ## Context *priv = ctx->priv_data; \ > + H26 ## h26n ## Raw ## ps_name *ps_var = unit->content; \ > unsigned int id = ps_var->id_element; \ > if (id > FF_ARRAY_ELEMS(priv->ps_var)) { \ > av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid " #ps_name \ > @@ -688,11 +689,11 @@ static int cbs_h26 ## h26n ## _replace_ ## > ps_var(CodedBitstreamContext *ctx, \ > } \ > if (priv->ps_var[id] == priv->active_ ## ps_var) \ > priv->active_ ## ps_var = NULL ; \ > - av_freep(&priv->ps_var[id]); \ > - priv->ps_var[id] = av_malloc(sizeof(*ps_var)); \ > - if (!priv->ps_var[id]) \ > + av_buffer_unref(&priv->ps_var ## _ref[id]); \ > + priv->ps_var ## _ref[id] = av_buffer_ref(unit->content_ref); \ > + if (!priv->ps_var ## _ref[id]) \ > return AVERROR(ENOMEM); \ > - memcpy(priv->ps_var[id], ps_var, sizeof(*ps_var)); \ > + priv->ps_var[id] = (H26 ## h26n ## Raw ## ps_name *)priv->ps_var ## > _ref[id]->data; \ > return 0; \ > } > > @@ -726,7 +727,7 @@ static int cbs_h264_read_nal_unit(CodedBitstreamContext > *ctx, > if (err < 0) > return err; > > - err = cbs_h264_replace_sps(ctx, sps); > + err = cbs_h264_replace_sps(ctx, unit); > if (err < 0) > return err; > } > @@ -760,7 +761,7 @@ static int cbs_h264_read_nal_unit(CodedBitstreamContext > *ctx, > if (err < 0) > return err; > > - err = cbs_h264_replace_pps(ctx, pps); > + err = cbs_h264_replace_pps(ctx, unit); > if (err < 0) > return err; > } > @@ -873,7 +874,7 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext > *ctx, > if (err < 0) > return err; > > - err = cbs_h265_replace_vps(ctx, vps); > + err = cbs_h265_replace_vps(ctx, unit); > if (err < 0) > return err; > } > @@ -892,7 +893,7 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext > *ctx, > if (err < 0) > return err; > > - err = cbs_h265_replace_sps(ctx, sps); > + err = cbs_h265_replace_sps(ctx, unit); > if (err < 0) > return err; > } > @@ -912,7 +913,7 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext > *ctx, > if (err < 0) > return err; > > - err = cbs_h265_replace_pps(ctx, pps); > + err = cbs_h265_replace_pps(ctx, unit); > if (err < 0) > return err; > } > @@ -1002,7 +1003,7 @@ static int > cbs_h264_write_nal_unit(CodedBitstreamContext *ctx, > if (err < 0) > return err; > > - err = cbs_h264_replace_sps(ctx, sps); > + err = cbs_h264_replace_sps(ctx, unit); > if (err < 0) > return err; > } > @@ -1026,7 +1027,7 @@ static int > cbs_h264_write_nal_unit(CodedBitstreamContext *ctx, > if (err < 0) > return err; > > - err = cbs_h264_replace_pps(ctx, pps); > + err = cbs_h264_replace_pps(ctx, unit); > if (err < 0) > return err; > } > @@ -1123,7 +1124,7 @@ static int > cbs_h265_write_nal_unit(CodedBitstreamContext *ctx, > if (err < 0) > return err; > > - err = cbs_h265_replace_vps(ctx, vps); > + err = cbs_h265_replace_vps(ctx, unit); > if (err < 0) > return err; > } > @@ -1137,7 +1138,7 @@ static int > cbs_h265_write_nal_unit(CodedBitstreamContext *ctx, > if (err < 0) > return err; > > - err = cbs_h265_replace_sps(ctx, sps); > + err = cbs_h265_replace_sps(ctx, unit); > if (err < 0) > return err; > } > @@ -1151,7 +1152,7 @@ static int > cbs_h265_write_nal_unit(CodedBitstreamContext *ctx, > if (err < 0) > return err; > > - err = cbs_h265_replace_pps(ctx, pps); > + err = cbs_h265_replace_pps(ctx, unit); > if (err < 0) > return err; > } > @@ -1385,9 +1386,9 @@ static void cbs_h264_close(CodedBitstreamContext *ctx) > av_freep(&h264->common.write_buffer); > > for (i = 0; i < FF_ARRAY_ELEMS(h264->sps); i++) > - av_freep(&h264->sps[i]); > + av_buffer_unref(&h264->sps_ref[i]); > for (i = 0; i < FF_ARRAY_ELEMS(h264->pps); i++) > - av_freep(&h264->pps[i]); > + av_buffer_unref(&h264->pps_ref[i]); > } > > static void cbs_h265_close(CodedBitstreamContext *ctx) > @@ -1400,11 +1401,11 @@ static void cbs_h265_close(CodedBitstreamContext *ctx) > av_freep(&h265->common.write_buffer); > > for (i = 0; i < FF_ARRAY_ELEMS(h265->vps); i++) > - av_freep(&h265->vps[i]); > + av_buffer_unref(&h265->vps_ref[i]); > for (i = 0; i < FF_ARRAY_ELEMS(h265->sps); i++) > - av_freep(&h265->sps[i]); > + av_buffer_unref(&h265->sps_ref[i]); > for (i = 0; i < FF_ARRAY_ELEMS(h265->pps); i++) > - av_freep(&h265->pps[i]); > + av_buffer_unref(&h265->pps_ref[i]); > } > > const CodedBitstreamType ff_cbs_type_h264 = { > diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h > index 33e71fc234..6d02979a17 100644 > --- a/libavcodec/cbs_h265.h > +++ b/libavcodec/cbs_h265.h > @@ -523,6 +523,9 @@ typedef struct CodedBitstreamH265Context { > > // All currently available parameter sets. These are updated when > // any parameter set NAL unit is read/written with this context. > + AVBufferRef *vps_ref[HEVC_MAX_VPS_COUNT]; > + AVBufferRef *sps_ref[HEVC_MAX_SPS_COUNT]; > + AVBufferRef *pps_ref[HEVC_MAX_PPS_COUNT]; > H265RawVPS *vps[HEVC_MAX_VPS_COUNT]; > H265RawSPS *sps[HEVC_MAX_SPS_COUNT]; > H265RawPPS *pps[HEVC_MAX_PPS_COUNT]; >
Ah. I should have thought of this problem earlier - testing this patch, it this breaks the VAAPI encoders because the parameter set structures there are not reference counted (and shouldn't be - they are pointers to substructures of the encoder context). $ gdb --args ./ffmpeg_g -y -i in.mp4 -an -vaapi_device /dev/dri/renderD128 -vf 'format=nv12,hwupload' -c:v h264_vaapi out.h264 ... Thread 1 "ffmpeg_g" received signal SIGSEGV, Segmentation fault. 0x00005555569bf65a in av_buffer_ref (buf=0x0) at src/libavutil/buffer.c:100 100 *ret = *buf; (gdb) bt #0 0x00005555569bf65a in av_buffer_ref (buf=0x0) at src/libavutil/buffer.c:100 #1 0x00005555564cd2ec in cbs_h264_replace_sps (ctx=0x555558dbee40, unit=0x555558dbde40) at src/libavcodec/cbs_h2645.c:700 #2 0x00005555564ce12f in cbs_h264_write_nal_unit (ctx=0x555558dbee40, unit=0x555558dbde40, pbc=0x7fffffffcf90) at src/libavcodec/cbs_h2645.c:1006 #3 0x00005555564ce943 in cbs_h2645_write_nal_unit (ctx=0x555558dbee40, unit=0x555558dbde40) at src/libavcodec/cbs_h2645.c:1258 #4 0x000055555649fd96 in ff_cbs_write_fragment_data (ctx=0x555558dbee40, frag=0x5555590da450) at src/libavcodec/cbs.c:284 #5 0x0000555556016784 in vaapi_encode_h264_write_access_unit (avctx=0x5555581ce800, data=0x7fffffffd0c0 "\020\321\377\377\377\177", data_len=0x7fffffffd4c0, au=0x5555590da450) at src/libavcodec/vaapi_encode_h264.c:109 #6 0x00005555560169f8 in vaapi_encode_h264_write_sequence_header (avctx=0x5555581ce800, data=0x7fffffffd0c0 "\020\321\377\377\377\177", data_len=0x7fffffffd4c0) at src/libavcodec/vaapi_encode_h264.c:171 #7 0x00005555566e87e3 in ff_vaapi_encode_init (avctx=0x5555581ce800) at src/libavcodec/vaapi_encode.c:1533 #8 0x00005555560194f1 in vaapi_encode_h264_init (avctx=0x5555581ce800) at src/libavcodec/vaapi_encode_h264.c:971 #9 0x0000555556006218 in avcodec_open2 (avctx=0x5555581ce800, codec=0x555557937300 <ff_h264_vaapi_encoder>, options=0x5555581d0a18) at src/libavcodec/utils.c:923 #10 0x000055555567a655 in init_output_stream (ost=0x5555581d08c0, error=0x7fffffffd8c0 "", error_len=1024) at src/fftools/ffmpeg.c:3485 #11 0x000055555567269e in reap_filters (flush=0) at src/fftools/ffmpeg.c:1442 #12 0x000055555567e88e in transcode_step () at src/fftools/ffmpeg.c:4587 #13 0x000055555567e955 in transcode () at src/fftools/ffmpeg.c:4631 #14 0x000055555567f1e5 in main (argc=12, argv=0x7fffffffe498) at src/fftools/ffmpeg.c:4838 Maybe if content_ref isn't set inside cbs_h26n_replace_xps() then make a new buffer with that reference? That still has the dangling pointer problem, though no current user will set any of those fields. Thanks, - Mark _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel