diff mbox series

[FFmpeg-devel,2/2] avcodec/hevcdec: sync User Data Unregistered SEI buffers across threads

Message ID 20200917133408.1113-2-jamrial@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/2] avcodec/hevcdec: sync SEI derived AVCodecContext fields across threads
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

James Almer Sept. 17, 2020, 1:34 p.m. UTC
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/hevcdec.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Guangxin Xu Sept. 17, 2020, 1:59 p.m. UTC | #1
On Thu, Sep 17, 2020 at 9:35 PM James Almer <jamrial@gmail.com> wrote:

> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/hevcdec.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index 1f3ea54d39..2481730788 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -3548,6 +3548,25 @@ static int
> hevc_update_thread_context(AVCodecContext *dst,
>              return AVERROR(ENOMEM);
>      }
>
> +    for (i = 0; i < s->sei.unregistered.nb_buf_ref; i++)
> +        av_buffer_unref(&s->sei.unregistered.buf_ref[i]);
> +    s->sei.unregistered.nb_buf_ref = 0;
>
Is it possible to define a new function ff_hevc_reset_sei_unregistered,
then we can reuse the function here and in ff_hevc_reset_sei?


> +
> +    if (s0->sei.unregistered.nb_buf_ref) {
> +        ret = av_reallocp_array(&s->sei.unregistered.buf_ref,
> +                                s0->sei.unregistered.nb_buf_ref,
> +                                sizeof(*s0->sei.unregistered.buf_ref));
>
This will call copy  &s->sei.unregistered.buf_ref,  but it's not needed.

+        if (ret < 0)
> +            return ret;
> +
> +        for (i = 0; i < s0->sei.unregistered.nb_buf_ref; i++) {
> +            s->sei.unregistered.buf_ref[i] =
> av_buffer_ref(s0->sei.unregistered.buf_ref[i]);
> +            if (!s->sei.unregistered.buf_ref[i])
> +                return AVERROR(ENOMEM);
> +            s->sei.unregistered.nb_buf_ref++;
> +        }
> +    }
> +
>      s->sei.frame_packing        = s0->sei.frame_packing;
>      s->sei.display_orientation  = s0->sei.display_orientation;
>      s->sei.mastering_display    = s0->sei.mastering_display;
> --
> 2.27.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer Sept. 17, 2020, 3:09 p.m. UTC | #2
On 9/17/2020 10:59 AM, Guangxin Xu wrote:
> On Thu, Sep 17, 2020 at 9:35 PM James Almer <jamrial@gmail.com> wrote:
> 
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/hevcdec.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
>> index 1f3ea54d39..2481730788 100644
>> --- a/libavcodec/hevcdec.c
>> +++ b/libavcodec/hevcdec.c
>> @@ -3548,6 +3548,25 @@ static int
>> hevc_update_thread_context(AVCodecContext *dst,
>>              return AVERROR(ENOMEM);
>>      }
>>
>> +    for (i = 0; i < s->sei.unregistered.nb_buf_ref; i++)
>> +        av_buffer_unref(&s->sei.unregistered.buf_ref[i]);
>> +    s->sei.unregistered.nb_buf_ref = 0;
>>
> Is it possible to define a new function ff_hevc_reset_sei_unregistered,
> then we can reuse the function here and in ff_hevc_reset_sei?

I don't think it's worth doing for only three lines.

> 
> 
>> +
>> +    if (s0->sei.unregistered.nb_buf_ref) {
>> +        ret = av_reallocp_array(&s->sei.unregistered.buf_ref,
>> +                                s0->sei.unregistered.nb_buf_ref,
>> +                                sizeof(*s0->sei.unregistered.buf_ref));
>>
> This will call copy  &s->sei.unregistered.buf_ref,  but it's not needed.

I made it this way since when both contexts have an allocated buffer of
the same size or smaller, the call will basically be a no-op and faster
than a free() + malloc().
I don't know how often that can be, considering the buffer will be freed
after generating an output frame, so I can change it before pushing if
you prefer the alternative.

> 
> +        if (ret < 0)
>> +            return ret;
>> +
>> +        for (i = 0; i < s0->sei.unregistered.nb_buf_ref; i++) {
>> +            s->sei.unregistered.buf_ref[i] =
>> av_buffer_ref(s0->sei.unregistered.buf_ref[i]);
>> +            if (!s->sei.unregistered.buf_ref[i])
>> +                return AVERROR(ENOMEM);
>> +            s->sei.unregistered.nb_buf_ref++;
>> +        }
>> +    }
>> +
>>      s->sei.frame_packing        = s0->sei.frame_packing;
>>      s->sei.display_orientation  = s0->sei.display_orientation;
>>      s->sei.mastering_display    = s0->sei.mastering_display;
>> --
>> 2.27.0
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Guangxin Xu Sept. 17, 2020, 3:49 p.m. UTC | #3
On Thu, Sep 17, 2020 at 11:09 PM James Almer <jamrial@gmail.com> wrote:

> On 9/17/2020 10:59 AM, Guangxin Xu wrote:
> > On Thu, Sep 17, 2020 at 9:35 PM James Almer <jamrial@gmail.com> wrote:
> >
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >>  libavcodec/hevcdec.c | 19 +++++++++++++++++++
> >>  1 file changed, 19 insertions(+)
> >>
> >> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> >> index 1f3ea54d39..2481730788 100644
> >> --- a/libavcodec/hevcdec.c
> >> +++ b/libavcodec/hevcdec.c
> >> @@ -3548,6 +3548,25 @@ static int
> >> hevc_update_thread_context(AVCodecContext *dst,
> >>              return AVERROR(ENOMEM);
> >>      }
> >>
> >> +    for (i = 0; i < s->sei.unregistered.nb_buf_ref; i++)
> >> +        av_buffer_unref(&s->sei.unregistered.buf_ref[i]);
> >> +    s->sei.unregistered.nb_buf_ref = 0;
> >>
> > Is it possible to define a new function ff_hevc_reset_sei_unregistered,
> > then we can reuse the function here and in ff_hevc_reset_sei?
>
> I don't think it's worth doing for only three lines.
>
> >
> >
> >> +
> >> +    if (s0->sei.unregistered.nb_buf_ref) {
> >> +        ret = av_reallocp_array(&s->sei.unregistered.buf_ref,
> >> +                                s0->sei.unregistered.nb_buf_ref,
> >> +                                sizeof(*s0->sei.unregistered.buf_ref));
> >>
> > This will call copy  &s->sei.unregistered.buf_ref,  but it's not needed.
>
> I made it this way since when both contexts have an allocated buffer of
> the same size or smaller, the call will basically be a no-op and faster
> than a free() + malloc().
>
In this case,  it's should be ok, thanks for explaining.


> I don't know how often that can be, considering the buffer will be freed
> after generating an output frame, so I can change it before pushing if
> you prefer the alternative.
>
> >
> > +        if (ret < 0)
> >> +            return ret;
> >> +
> >> +        for (i = 0; i < s0->sei.unregistered.nb_buf_ref; i++) {
> >> +            s->sei.unregistered.buf_ref[i] =
> >> av_buffer_ref(s0->sei.unregistered.buf_ref[i]);
> >> +            if (!s->sei.unregistered.buf_ref[i])
> >> +                return AVERROR(ENOMEM);
> >> +            s->sei.unregistered.nb_buf_ref++;
> >> +        }
> >> +    }
> >> +
> >>      s->sei.frame_packing        = s0->sei.frame_packing;
> >>      s->sei.display_orientation  = s0->sei.display_orientation;
> >>      s->sei.mastering_display    = s0->sei.mastering_display;
> >> --
> >> 2.27.0
> >>
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> To unsubscribe, visit link above, or email
> >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index 1f3ea54d39..2481730788 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -3548,6 +3548,25 @@  static int hevc_update_thread_context(AVCodecContext *dst,
             return AVERROR(ENOMEM);
     }
 
+    for (i = 0; i < s->sei.unregistered.nb_buf_ref; i++)
+        av_buffer_unref(&s->sei.unregistered.buf_ref[i]);
+    s->sei.unregistered.nb_buf_ref = 0;
+
+    if (s0->sei.unregistered.nb_buf_ref) {
+        ret = av_reallocp_array(&s->sei.unregistered.buf_ref,
+                                s0->sei.unregistered.nb_buf_ref,
+                                sizeof(*s0->sei.unregistered.buf_ref));
+        if (ret < 0)
+            return ret;
+
+        for (i = 0; i < s0->sei.unregistered.nb_buf_ref; i++) {
+            s->sei.unregistered.buf_ref[i] = av_buffer_ref(s0->sei.unregistered.buf_ref[i]);
+            if (!s->sei.unregistered.buf_ref[i])
+                return AVERROR(ENOMEM);
+            s->sei.unregistered.nb_buf_ref++;
+        }
+    }
+
     s->sei.frame_packing        = s0->sei.frame_packing;
     s->sei.display_orientation  = s0->sei.display_orientation;
     s->sei.mastering_display    = s0->sei.mastering_display;