diff mbox

[FFmpeg-devel,2/3] h2645_parse: Make ff_h2645_packet_split reference-compatible

Message ID 20181121183431.6404-3-andreas.rheinhardt@googlemail.com
State Accepted
Headers show

Commit Message

Andreas Rheinhardt Nov. 21, 2018, 6:34 p.m. UTC 
This is in preparation for a patch for cbs_h2645. Now the packet's
rbsp_buffer can be owned by an AVBuffer.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com>
---
 libavcodec/cbs_h2645.c             | 12 ++++-----
 libavcodec/extract_extradata_bsf.c |  4 +--
 libavcodec/h2645_parse.c           | 43 +++++++++++++++++++++++++++---
 libavcodec/h2645_parse.h           | 10 +++++--
 libavcodec/h264_parse.c            |  4 +--
 libavcodec/h264dec.c               |  6 ++---
 libavcodec/hevc_parse.c            |  5 ++--
 libavcodec/hevc_parser.c           |  4 +--
 libavcodec/hevcdec.c               |  4 +--
 9 files changed, 67 insertions(+), 25 deletions(-)

Comments

Mark Thompson Nov. 22, 2018, 11:35 p.m. UTC | #1 
On 21/11/18 18:34, Andreas Rheinhardt wrote:
> This is in preparation for a patch for cbs_h2645. Now the packet's
> rbsp_buffer can be owned by an AVBuffer.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com>
> ---
>  libavcodec/cbs_h2645.c             | 12 ++++-----
>  libavcodec/extract_extradata_bsf.c |  4 +--
>  libavcodec/h2645_parse.c           | 43 +++++++++++++++++++++++++++---
>  libavcodec/h2645_parse.h           | 10 +++++--
>  libavcodec/h264_parse.c            |  4 +--
>  libavcodec/h264dec.c               |  6 ++---
>  libavcodec/hevc_parse.c            |  5 ++--
>  libavcodec/hevc_parser.c           |  4 +--
>  libavcodec/hevcdec.c               |  4 +--
>  9 files changed, 67 insertions(+), 25 deletions(-)
> 
> ...
> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> index aaa4b8f443..ad5d04f0a4 100644
> --- a/libavcodec/h2645_parse.c
> +++ b/libavcodec/h2645_parse.c
> @@ -343,9 +343,37 @@ static int find_next_start_code(const uint8_t *buf, const uint8_t *next_avc)
>      return i + 3;
>  }
>  
> +static void handle_ref_buffer(H2645RBSP *rbsp, AVBufferRef **ref, int size)
> +{
> +    if (size + AV_INPUT_BUFFER_PADDING_SIZE <= 0)
> +        goto fail;
> +
> +    size += AV_INPUT_BUFFER_PADDING_SIZE;
> +
> +    if (*ref && (*ref)->size >= size && av_buffer_is_writable(*ref))
> +        return;
> +
> +    av_buffer_unref(ref);
> +
> +    size = FFMAX(size + (size >> 4) + 32, size);
> +
> +    *ref = av_buffer_alloc(size);
> +    if (*ref) {
> +        rbsp->rbsp_buffer            = (*ref)->data;
> +        rbsp->rbsp_buffer_alloc_size = size;
> +        return;
> +    }
> +
> +fail:
> +    av_buffer_unref(ref);
> +    rbsp->rbsp_buffer            = NULL;
> +    rbsp->rbsp_buffer_alloc_size = 0;
> +    return;
> +}
> +
>  int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>                            void *logctx, int is_nalff, int nal_length_size,
> -                          enum AVCodecID codec_id, int small_padding)
> +                          enum AVCodecID codec_id, int small_padding, AVBufferRef **ref)
>  {
>      GetByteContext bc;
>      int consumed, ret = 0;
> @@ -353,7 +381,11 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>      int64_t padding = small_padding ? 0 : MAX_MBPAIR_SIZE;
>  
>      bytestream2_init(&bc, buf, length);
> -    av_fast_padded_malloc(&pkt->rbsp.rbsp_buffer, &pkt->rbsp.rbsp_buffer_alloc_size, length + padding);
> +    if (!ref)
> +        av_fast_padded_malloc(&pkt->rbsp.rbsp_buffer, &pkt->rbsp.rbsp_buffer_alloc_size, length + padding);
> +    else
> +        handle_ref_buffer(&pkt->rbsp, ref, length + padding);
> +
>      if (!pkt->rbsp.rbsp_buffer)
>          return AVERROR(ENOMEM);
>  
> @@ -466,7 +498,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>      return 0;
>  }
>  
> -void ff_h2645_packet_uninit(H2645Packet *pkt)
> +void ff_h2645_packet_uninit(H2645Packet *pkt, AVBufferRef *ref)
>  {
>      int i;
>      for (i = 0; i < pkt->nals_allocated; i++) {
> @@ -474,6 +506,9 @@ void ff_h2645_packet_uninit(H2645Packet *pkt)
>      }
>      av_freep(&pkt->nals);
>      pkt->nals_allocated = 0;
> -    av_freep(&pkt->rbsp.rbsp_buffer);
> +    if (!ref)
> +        av_freep(&pkt->rbsp.rbsp_buffer);
> +    else
> +        pkt->rbsp.rbsp_buffer = NULL;
>      pkt->rbsp.rbsp_buffer_alloc_size = pkt->rbsp.rbsp_buffer_size = 0;
>  }

I don't think I like how you've ended up with this dummy argument to uninit saying whether the content should be freed or not.

Can we instead move the AVBufferRef into the H2645Packet structure itself, so the user doesn't have to separately keep track of it?  Since the same packet structure is reused in cbs_h2645.c without an uninit, I think that avoids having to pass an extra argument to either function.

Thanks,

- Mark
Andreas Rheinhardt Nov. 28, 2018, 12:24 a.m. UTC | #2 
I have incorporated your proposal and have added the buffer directly to
H2645RBSP. This allowed to simplify the uninit process, as you already
predicted.


Andreas Rheinhardt (2):
  h2645_parse: Make ff_h2645_packet_split reference-compatible
  cbs_h2645: Avoid memcpy when splitting fragment #2

 libavcodec/cbs_h2645.c             | 33 +++++++------------
 libavcodec/extract_extradata_bsf.c |  2 +-
 libavcodec/h2645_parse.c           | 53 ++++++++++++++++++++++++++++--
 libavcodec/h2645_parse.h           |  9 ++++-
 libavcodec/h264_parse.c            |  2 +-
 libavcodec/h264dec.c               |  4 +--
 libavcodec/hevc_parse.c            |  3 +-
 libavcodec/hevc_parser.c           |  2 +-
 libavcodec/hevcdec.c               |  2 +-
 9 files changed, 77 insertions(+), 33 deletions(-)
Mark Thompson Jan. 23, 2019, 10:44 p.m. UTC | #3 
On 28/11/2018 00:24, Andreas Rheinhardt wrote:
> I have incorporated your proposal and have added the buffer directly to
> H2645RBSP. This allowed to simplify the uninit process, as you already
> predicted.

I don't much like the implicit distinction between buffers being refcounted or not in this, but I don't see a better way to do it without changing the API completely.

Otherwise all looks good, so applied.

Thanks,

- Mark
diff mbox

Patch

diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
index 6846cad0bb..df2b5f3f5e 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -611,7 +611,7 @@  static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
 
         err = ff_h2645_packet_split(&priv->read_packet,
                                     frag->data + start, end - start,
-                                    ctx->log_ctx, 1, 2, AV_CODEC_ID_H264, 1);
+                                    ctx->log_ctx, 1, 2, AV_CODEC_ID_H264, 1, NULL);
         if (err < 0) {
             av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to split AVCC SPS array.\n");
             return err;
@@ -635,7 +635,7 @@  static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
 
         err = ff_h2645_packet_split(&priv->read_packet,
                                     frag->data + start, end - start,
-                                    ctx->log_ctx, 1, 2, AV_CODEC_ID_H264, 1);
+                                    ctx->log_ctx, 1, 2, AV_CODEC_ID_H264, 1, NULL);
         if (err < 0) {
             av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to split AVCC PPS array.\n");
             return err;
@@ -689,7 +689,7 @@  static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
 
             err = ff_h2645_packet_split(&priv->read_packet,
                                         frag->data + start, end - start,
-                                        ctx->log_ctx, 1, 2, AV_CODEC_ID_HEVC, 1);
+                                        ctx->log_ctx, 1, 2, AV_CODEC_ID_HEVC, 1, NULL);
             if (err < 0) {
                 av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to split "
                        "HVCC array %d (%d NAL units of type %d).\n",
@@ -708,7 +708,7 @@  static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
                                     frag->data, frag->data_size,
                                     ctx->log_ctx,
                                     priv->mp4, priv->nal_length_size,
-                                    codec_id, 1);
+                                    codec_id, 1, NULL);
         if (err < 0)
             return err;
 
@@ -1510,7 +1510,7 @@  static void cbs_h264_close(CodedBitstreamContext *ctx)
     CodedBitstreamH264Context *h264 = ctx->priv_data;
     int i;
 
-    ff_h2645_packet_uninit(&h264->common.read_packet);
+    ff_h2645_packet_uninit(&h264->common.read_packet, NULL);
 
     av_freep(&h264->common.write_buffer);
 
@@ -1525,7 +1525,7 @@  static void cbs_h265_close(CodedBitstreamContext *ctx)
     CodedBitstreamH265Context *h265 = ctx->priv_data;
     int i;
 
-    ff_h2645_packet_uninit(&h265->common.read_packet);
+    ff_h2645_packet_uninit(&h265->common.read_packet, NULL);
 
     av_freep(&h265->common.write_buffer);
 
diff --git a/libavcodec/extract_extradata_bsf.c b/libavcodec/extract_extradata_bsf.c
index f37427c7e1..5598064eb3 100644
--- a/libavcodec/extract_extradata_bsf.c
+++ b/libavcodec/extract_extradata_bsf.c
@@ -157,7 +157,7 @@  static int extract_extradata_h2645(AVBSFContext *ctx, AVPacket *pkt,
     }
 
     ret = ff_h2645_packet_split(&s->h2645_pkt, pkt->data, pkt->size,
-                                ctx, 0, 0, ctx->par_in->codec_id, 1);
+                                ctx, 0, 0, ctx->par_in->codec_id, 1, NULL);
     if (ret < 0)
         return ret;
 
@@ -393,7 +393,7 @@  static void extract_extradata_close(AVBSFContext *ctx)
 {
     ExtractExtradataContext *s = ctx->priv_data;
     ff_av1_packet_uninit(&s->av1_pkt);
-    ff_h2645_packet_uninit(&s->h2645_pkt);
+    ff_h2645_packet_uninit(&s->h2645_pkt, NULL);
 }
 
 static const enum AVCodecID codec_ids[] = {
diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
index aaa4b8f443..ad5d04f0a4 100644
--- a/libavcodec/h2645_parse.c
+++ b/libavcodec/h2645_parse.c
@@ -343,9 +343,37 @@  static int find_next_start_code(const uint8_t *buf, const uint8_t *next_avc)
     return i + 3;
 }
 
+static void handle_ref_buffer(H2645RBSP *rbsp, AVBufferRef **ref, int size)
+{
+    if (size + AV_INPUT_BUFFER_PADDING_SIZE <= 0)
+        goto fail;
+
+    size += AV_INPUT_BUFFER_PADDING_SIZE;
+
+    if (*ref && (*ref)->size >= size && av_buffer_is_writable(*ref))
+        return;
+
+    av_buffer_unref(ref);
+
+    size = FFMAX(size + (size >> 4) + 32, size);
+
+    *ref = av_buffer_alloc(size);
+    if (*ref) {
+        rbsp->rbsp_buffer            = (*ref)->data;
+        rbsp->rbsp_buffer_alloc_size = size;
+        return;
+    }
+
+fail:
+    av_buffer_unref(ref);
+    rbsp->rbsp_buffer            = NULL;
+    rbsp->rbsp_buffer_alloc_size = 0;
+    return;
+}
+
 int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
                           void *logctx, int is_nalff, int nal_length_size,
-                          enum AVCodecID codec_id, int small_padding)
+                          enum AVCodecID codec_id, int small_padding, AVBufferRef **ref)
 {
     GetByteContext bc;
     int consumed, ret = 0;
@@ -353,7 +381,11 @@  int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
     int64_t padding = small_padding ? 0 : MAX_MBPAIR_SIZE;
 
     bytestream2_init(&bc, buf, length);
-    av_fast_padded_malloc(&pkt->rbsp.rbsp_buffer, &pkt->rbsp.rbsp_buffer_alloc_size, length + padding);
+    if (!ref)
+        av_fast_padded_malloc(&pkt->rbsp.rbsp_buffer, &pkt->rbsp.rbsp_buffer_alloc_size, length + padding);
+    else
+        handle_ref_buffer(&pkt->rbsp, ref, length + padding);
+
     if (!pkt->rbsp.rbsp_buffer)
         return AVERROR(ENOMEM);
 
@@ -466,7 +498,7 @@  int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
     return 0;
 }
 
-void ff_h2645_packet_uninit(H2645Packet *pkt)
+void ff_h2645_packet_uninit(H2645Packet *pkt, AVBufferRef *ref)
 {
     int i;
     for (i = 0; i < pkt->nals_allocated; i++) {
@@ -474,6 +506,9 @@  void ff_h2645_packet_uninit(H2645Packet *pkt)
     }
     av_freep(&pkt->nals);
     pkt->nals_allocated = 0;
-    av_freep(&pkt->rbsp.rbsp_buffer);
+    if (!ref)
+        av_freep(&pkt->rbsp.rbsp_buffer);
+    else
+        pkt->rbsp.rbsp_buffer = NULL;
     pkt->rbsp.rbsp_buffer_alloc_size = pkt->rbsp.rbsp_buffer_size = 0;
 }
diff --git a/libavcodec/h2645_parse.h b/libavcodec/h2645_parse.h
index a0a5ca5868..bdd8a32466 100644
--- a/libavcodec/h2645_parse.h
+++ b/libavcodec/h2645_parse.h
@@ -23,6 +23,7 @@ 
 
 #include <stdint.h>
 
+#include "libavutil/buffer.h"
 #include "avcodec.h"
 #include "get_bits.h"
 
@@ -91,15 +92,20 @@  int ff_h2645_extract_rbsp(const uint8_t *src, int length, H2645RBSP *rbsp,
  * the data is contained in the input buffer pointed to by buf.
  * Otherwise, the unescaped data is part of the rbsp_buffer described by the
  * packet's H2645RBSP.
+ * If ref is not NULL, *ref must either be NULL in which case pkt's underlying
+ * rbsp_buffer must be NULL, too, or (*ref)->buffer owns rbsp_buffer. In both
+ * cases, rbsp_buffer will be owned by **ref afterwards.
  */
 int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
                           void *logctx, int is_nalff, int nal_length_size,
-                          enum AVCodecID codec_id, int small_padding);
+                          enum AVCodecID codec_id, int small_padding, AVBufferRef **ref);
 
 /**
  * Free all the allocated memory in the packet.
+ * If ref is supplied, the underlying AVBuffer is supposed to own the packet's
+ * rbsp_buffer.
  */
-void ff_h2645_packet_uninit(H2645Packet *pkt);
+void ff_h2645_packet_uninit(H2645Packet *pkt, AVBufferRef *ref);
 
 static inline int get_nalsize(int nal_length_size, const uint8_t *buf,
                               int buf_size, int *buf_index, void *logctx)
diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
index 34ffe3b1fe..57f0e8692e 100644
--- a/libavcodec/h264_parse.c
+++ b/libavcodec/h264_parse.c
@@ -359,7 +359,7 @@  static int decode_extradata_ps(const uint8_t *data, int size, H264ParamSets *ps,
     H2645Packet pkt = { 0 };
     int i, ret = 0;
 
-    ret = ff_h2645_packet_split(&pkt, data, size, logctx, is_avc, 2, AV_CODEC_ID_H264, 1);
+    ret = ff_h2645_packet_split(&pkt, data, size, logctx, is_avc, 2, AV_CODEC_ID_H264, 1, NULL);
     if (ret < 0) {
         ret = 0;
         goto fail;
@@ -387,7 +387,7 @@  static int decode_extradata_ps(const uint8_t *data, int size, H264ParamSets *ps,
     }
 
 fail:
-    ff_h2645_packet_uninit(&pkt);
+    ff_h2645_packet_uninit(&pkt, NULL);
     return ret;
 }
 
diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
index 00d922fbe9..62a25eda31 100644
--- a/libavcodec/h264dec.c
+++ b/libavcodec/h264dec.c
@@ -377,7 +377,7 @@  static av_cold int h264_decode_end(AVCodecContext *avctx)
     ff_h264_sei_uninit(&h->sei);
     ff_h264_ps_uninit(&h->ps);
 
-    ff_h2645_packet_uninit(&h->pkt);
+    ff_h2645_packet_uninit(&h->pkt, NULL);
 
     ff_h264_unref_picture(h, &h->cur_pic);
     av_frame_free(&h->cur_pic.f);
@@ -622,8 +622,8 @@  static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size)
             h->is_avc = 1;
     }
 
-    ret = ff_h2645_packet_split(&h->pkt, buf, buf_size, avctx, h->is_avc,
-                                h->nal_length_size, avctx->codec_id, avctx->flags2 & AV_CODEC_FLAG2_FAST);
+    ret = ff_h2645_packet_split(&h->pkt, buf, buf_size, avctx, h->is_avc, h->nal_length_size,
+                                avctx->codec_id, avctx->flags2 & AV_CODEC_FLAG2_FAST, NULL);
     if (ret < 0) {
         av_log(avctx, AV_LOG_ERROR,
                "Error splitting the input into NAL units.\n");
diff --git a/libavcodec/hevc_parse.c b/libavcodec/hevc_parse.c
index b1b27eef09..3cf9e3757e 100644
--- a/libavcodec/hevc_parse.c
+++ b/libavcodec/hevc_parse.c
@@ -29,7 +29,8 @@  static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets
     int ret = 0;
     H2645Packet pkt = { 0 };
 
-    ret = ff_h2645_packet_split(&pkt, buf, buf_size, logctx, is_nalff, nal_length_size, AV_CODEC_ID_HEVC, 1);
+    ret = ff_h2645_packet_split(&pkt, buf, buf_size, logctx, is_nalff,
+                                nal_length_size, AV_CODEC_ID_HEVC, 1, NULL);
     if (ret < 0) {
         goto done;
     }
@@ -67,7 +68,7 @@  static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets
     }
 
 done:
-    ff_h2645_packet_uninit(&pkt);
+    ff_h2645_packet_uninit(&pkt, NULL);
     if (err_recognition & AV_EF_EXPLODE)
         return ret;
 
diff --git a/libavcodec/hevc_parser.c b/libavcodec/hevc_parser.c
index 369d1338d0..51c240069d 100644
--- a/libavcodec/hevc_parser.c
+++ b/libavcodec/hevc_parser.c
@@ -194,7 +194,7 @@  static int parse_nal_units(AVCodecParserContext *s, const uint8_t *buf,
     ff_hevc_reset_sei(sei);
 
     ret = ff_h2645_packet_split(&ctx->pkt, buf, buf_size, avctx, ctx->is_avc,
-                                ctx->nal_length_size, AV_CODEC_ID_HEVC, 1);
+                                ctx->nal_length_size, AV_CODEC_ID_HEVC, 1, NULL);
     if (ret < 0)
         return ret;
 
@@ -363,7 +363,7 @@  static void hevc_parser_close(AVCodecParserContext *s)
     HEVCParserContext *ctx = s->priv_data;
 
     ff_hevc_ps_uninit(&ctx->ps);
-    ff_h2645_packet_uninit(&ctx->pkt);
+    ff_h2645_packet_uninit(&ctx->pkt, NULL);
     ff_hevc_reset_sei(&ctx->sei);
 
     av_freep(&ctx->pc.buffer);
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index a3b5c8cb71..42fdb090ec 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -3024,7 +3024,7 @@  static int decode_nal_units(HEVCContext *s, const uint8_t *buf, int length)
     /* split the input packet into NAL units, so we know the upper bound on the
      * number of slices in the frame */
     ret = ff_h2645_packet_split(&s->pkt, buf, length, s->avctx, s->is_nalff,
-                                s->nal_length_size, s->avctx->codec_id, 1);
+                                s->nal_length_size, s->avctx->codec_id, 1, NULL);
     if (ret < 0) {
         av_log(s->avctx, AV_LOG_ERROR,
                "Error splitting the input into NAL units.\n");
@@ -3305,7 +3305,7 @@  static av_cold int hevc_decode_free(AVCodecContext *avctx)
         s->HEVClc = NULL;
     av_freep(&s->HEVClcList[0]);
 
-    ff_h2645_packet_uninit(&s->pkt);
+    ff_h2645_packet_uninit(&s->pkt, NULL);
 
     return 0;
 }