| Message ID | 20180508204856.10796-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 08/05/18 21:48, James Almer wrote: > There's no gain from using AVBufferRef for these, as no copies or > references are ever made. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > There is however a raw copy of the struct storing these buffers, > which is dangerous and fragile. > This patch is in preparation to change how the above is handled. > > libavcodec/cbs_h264.h | 1 - > libavcodec/cbs_h2645.c | 13 ++++++------- > libavcodec/cbs_h265.h | 1 - > 3 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h > index 2219d9da8d..becea3adfe 100644 > --- a/libavcodec/cbs_h264.h > +++ b/libavcodec/cbs_h264.h > @@ -197,7 +197,6 @@ typedef struct H264RawPPS { > uint16_t pic_size_in_map_units_minus1; > > uint8_t *slice_group_id; > - AVBufferRef *slice_group_id_ref; > > uint8_t num_ref_idx_l0_default_active_minus1; > uint8_t num_ref_idx_l1_default_active_minus1; > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c > index 64a1a2d1ee..580ca09f63 100644 > --- a/libavcodec/cbs_h2645.c > +++ b/libavcodec/cbs_h2645.c > @@ -318,10 +318,9 @@ static int cbs_h2645_read_more_rbsp_data(GetBitContext *gbc) > #define byte_alignment(rw) (get_bits_count(rw) % 8) > > #define allocate(name, size) do { \ > - name ## _ref = av_buffer_allocz(size); \ > - if (!name ## _ref) \ > + name = av_mallocz(size); \ > + if (!name) \ > return AVERROR(ENOMEM); \ > - name = name ## _ref->data; \ > } while (0) This breaks other users of this macro (H.264 SEI). The reason for using the bufferref here is not really that you might want to make more references to it. Rather, it is for the alloc/free properties which give control to the user - for example, they can set one of these pointers to some internal static buffer they hold while setting _ref to null, and the free code still does the right thing (i.e. nothing). I don't think that argument will necessarily apply to any of the values changed here - I doubt anyone will ever touch the FMO slice_group_id, and the H.265 PS extension data bits will need to have defined meanings (and therefore moved out of the "unknown future stuff" case) before they gets used. Still, I'm not sure how you've gained anything - since the PS objects are refcounted in the following patch, how does this change actually help? - Mark
On 5/8/2018 7:17 PM, Mark Thompson wrote: > On 08/05/18 21:48, James Almer wrote: >> There's no gain from using AVBufferRef for these, as no copies or >> references are ever made. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> There is however a raw copy of the struct storing these buffers, >> which is dangerous and fragile. >> This patch is in preparation to change how the above is handled. >> >> libavcodec/cbs_h264.h | 1 - >> libavcodec/cbs_h2645.c | 13 ++++++------- >> libavcodec/cbs_h265.h | 1 - >> 3 files changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h >> index 2219d9da8d..becea3adfe 100644 >> --- a/libavcodec/cbs_h264.h >> +++ b/libavcodec/cbs_h264.h >> @@ -197,7 +197,6 @@ typedef struct H264RawPPS { >> uint16_t pic_size_in_map_units_minus1; >> >> uint8_t *slice_group_id; >> - AVBufferRef *slice_group_id_ref; >> >> uint8_t num_ref_idx_l0_default_active_minus1; >> uint8_t num_ref_idx_l1_default_active_minus1; >> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c >> index 64a1a2d1ee..580ca09f63 100644 >> --- a/libavcodec/cbs_h2645.c >> +++ b/libavcodec/cbs_h2645.c >> @@ -318,10 +318,9 @@ static int cbs_h2645_read_more_rbsp_data(GetBitContext *gbc) >> #define byte_alignment(rw) (get_bits_count(rw) % 8) >> >> #define allocate(name, size) do { \ >> - name ## _ref = av_buffer_allocz(size); \ >> - if (!name ## _ref) \ >> + name = av_mallocz(size); \ >> + if (!name) \ >> return AVERROR(ENOMEM); \ >> - name = name ## _ref->data; \ >> } while (0) > > This breaks other users of this macro (H.264 SEI). > > The reason for using the bufferref here is not really that you might want to make more references to it. Rather, it is for the alloc/free properties which give control to the user - for example, they can set one of these pointers to some internal static buffer they hold while setting _ref to null, and the free code still does the right thing (i.e. nothing). > > I don't think that argument will necessarily apply to any of the values changed here - I doubt anyone will ever touch the FMO slice_group_id, and the H.265 PS extension data bits will need to have defined meanings (and therefore moved out of the "unknown future stuff" case) before they gets used. Still, I'm not sure how you've gained anything - since the PS objects are refcounted in the following patch, how does this change actually help? Removing the overhead of having these be AVBufferRef instead of a simple malloc'ed array, since at least after a quick glance i couldn't find any reason for it. If you think keeping them as AVBufferRef is better/future proof then we can drop this. The next patch works around the issue of memcpy'ing them anyway. > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 08/05/18 23:36, James Almer wrote: > On 5/8/2018 7:17 PM, Mark Thompson wrote: >> On 08/05/18 21:48, James Almer wrote: >>> There's no gain from using AVBufferRef for these, as no copies or >>> references are ever made. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> There is however a raw copy of the struct storing these buffers, >>> which is dangerous and fragile. >>> This patch is in preparation to change how the above is handled. >>> >>> libavcodec/cbs_h264.h | 1 - >>> libavcodec/cbs_h2645.c | 13 ++++++------- >>> libavcodec/cbs_h265.h | 1 - >>> 3 files changed, 6 insertions(+), 9 deletions(-) >>> >>> diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h >>> index 2219d9da8d..becea3adfe 100644 >>> --- a/libavcodec/cbs_h264.h >>> +++ b/libavcodec/cbs_h264.h >>> @@ -197,7 +197,6 @@ typedef struct H264RawPPS { >>> uint16_t pic_size_in_map_units_minus1; >>> >>> uint8_t *slice_group_id; >>> - AVBufferRef *slice_group_id_ref; >>> >>> uint8_t num_ref_idx_l0_default_active_minus1; >>> uint8_t num_ref_idx_l1_default_active_minus1; >>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c >>> index 64a1a2d1ee..580ca09f63 100644 >>> --- a/libavcodec/cbs_h2645.c >>> +++ b/libavcodec/cbs_h2645.c >>> @@ -318,10 +318,9 @@ static int cbs_h2645_read_more_rbsp_data(GetBitContext *gbc) >>> #define byte_alignment(rw) (get_bits_count(rw) % 8) >>> >>> #define allocate(name, size) do { \ >>> - name ## _ref = av_buffer_allocz(size); \ >>> - if (!name ## _ref) \ >>> + name = av_mallocz(size); \ >>> + if (!name) \ >>> return AVERROR(ENOMEM); \ >>> - name = name ## _ref->data; \ >>> } while (0) >> >> This breaks other users of this macro (H.264 SEI). >> >> The reason for using the bufferref here is not really that you might want to make more references to it. Rather, it is for the alloc/free properties which give control to the user - for example, they can set one of these pointers to some internal static buffer they hold while setting _ref to null, and the free code still does the right thing (i.e. nothing). >> >> I don't think that argument will necessarily apply to any of the values changed here - I doubt anyone will ever touch the FMO slice_group_id, and the H.265 PS extension data bits will need to have defined meanings (and therefore moved out of the "unknown future stuff" case) before they gets used. Still, I'm not sure how you've gained anything - since the PS objects are refcounted in the following patch, how does this change actually help? > > Removing the overhead of having these be AVBufferRef instead of a simple > malloc'ed array, since at least after a quick glance i couldn't find any > reason for it. The overhead will almost always be zero - none of these fields will ever appear in a "normal" stream (nobody uses FMO except to catch out people who think they support H.264 baseline profile, and as further H.265 parameter set extension bits are defined we will want to add them to be parsed explicitly). > If you think keeping them as AVBufferRef is better/future proof then we > can drop this. The next patch works around the issue of memcpy'ing them > anyway. So, unless you feel strongly I think it would be easier to keep treating them in the same way as the SEI data buffers, which do want this behaviour. Thanks, - Mark
diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h index 2219d9da8d..becea3adfe 100644 --- a/libavcodec/cbs_h264.h +++ b/libavcodec/cbs_h264.h @@ -197,7 +197,6 @@ typedef struct H264RawPPS { uint16_t pic_size_in_map_units_minus1; uint8_t *slice_group_id; - AVBufferRef *slice_group_id_ref; uint8_t num_ref_idx_l0_default_active_minus1; uint8_t num_ref_idx_l1_default_active_minus1; diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c index 64a1a2d1ee..580ca09f63 100644 --- a/libavcodec/cbs_h2645.c +++ b/libavcodec/cbs_h2645.c @@ -318,10 +318,9 @@ static int cbs_h2645_read_more_rbsp_data(GetBitContext *gbc) #define byte_alignment(rw) (get_bits_count(rw) % 8) #define allocate(name, size) do { \ - name ## _ref = av_buffer_allocz(size); \ - if (!name ## _ref) \ + name = av_mallocz(size); \ + if (!name) \ return AVERROR(ENOMEM); \ - name = name ## _ref->data; \ } while (0) #define FUNC(name) FUNC_H264(READWRITE, name) @@ -415,7 +414,7 @@ static int cbs_h2645_read_more_rbsp_data(GetBitContext *gbc) static void cbs_h264_free_pps(void *unit, uint8_t *content) { H264RawPPS *pps = (H264RawPPS*)content; - av_buffer_unref(&pps->slice_group_id_ref); + av_free(pps->slice_group_id); av_freep(&content); } @@ -458,21 +457,21 @@ static void cbs_h264_free_slice(void *unit, uint8_t *content) static void cbs_h265_free_vps(void *unit, uint8_t *content) { H265RawVPS *vps = (H265RawVPS*)content; - av_buffer_unref(&vps->extension_data.data_ref); + av_free(vps->extension_data.data); av_freep(&content); } static void cbs_h265_free_sps(void *unit, uint8_t *content) { H265RawSPS *sps = (H265RawSPS*)content; - av_buffer_unref(&sps->extension_data.data_ref); + av_free(sps->extension_data.data); av_freep(&content); } static void cbs_h265_free_pps(void *unit, uint8_t *content) { H265RawPPS *pps = (H265RawPPS*)content; - av_buffer_unref(&pps->extension_data.data_ref); + av_free(pps->extension_data.data); av_freep(&content); } diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h index 33e71fc234..1b357293ab 100644 --- a/libavcodec/cbs_h265.h +++ b/libavcodec/cbs_h265.h @@ -154,7 +154,6 @@ typedef struct H265RawVUI { typedef struct H265RawPSExtensionData { uint8_t *data; size_t bit_length; - AVBufferRef *data_ref; } H265RawPSExtensionData; typedef struct H265RawVPS {
There's no gain from using AVBufferRef for these, as no copies or references are ever made. Signed-off-by: James Almer <jamrial@gmail.com> --- There is however a raw copy of the struct storing these buffers, which is dangerous and fragile. This patch is in preparation to change how the above is handled. libavcodec/cbs_h264.h | 1 - libavcodec/cbs_h2645.c | 13 ++++++------- libavcodec/cbs_h265.h | 1 - 3 files changed, 6 insertions(+), 9 deletions(-)