diff mbox series

[FFmpeg-devel] avcodec/qsv_enc: do not reuse enc_ctrl from previous frames

Message ID 20210106031234.13435-1-guangxin.xu@intel.com
State Accepted
Commit 3bbe0c210b05fc6fbd7b1d4bbd8479db7f2cf957
Headers show
Series [FFmpeg-devel] avcodec/qsv_enc: do not reuse enc_ctrl from previous frames | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Xu, Guangxin Jan. 6, 2021, 3:12 a.m. UTC 
fixes #8857

If we do not clear the enc_ctrl, we will reuse previous frames' data like FrameType.
---
 libavcodec/qsvenc.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Xiang, Haihao Jan. 6, 2021, 6:53 a.m. UTC | #1 
On Wed, 2021-01-06 at 11:12 +0800, Xu Guangxin wrote:
> fixes #8857
> 
> If we do not clear the enc_ctrl, we will reuse previous frames' data like
> FrameType.
> ---
>  libavcodec/qsvenc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index 2bd2a56227..94473c4eab 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1249,6 +1249,8 @@ static void clear_unused_frames(QSVEncContext *q)
>      while (cur) {
>          if (cur->used && !cur->surface.Data.Locked) {
>              free_encoder_ctrl_payloads(&cur->enc_ctrl);
> +            //do not reuse enc_ctrl from previous frame
> +            memset(&cur->enc_ctrl, 0, sizeof(cur->enc_ctrl));
>              if (cur->frame->format == AV_PIX_FMT_QSV) {
>                  av_frame_unref(cur->frame);
>              }

LGTM

Thanks
Haihao
Xiang, Haihao Sept. 9, 2021, 2:33 p.m. UTC | #2 
On Wed, 2021-01-06 at 06:53 +0000, Xiang, Haihao wrote:
> On Wed, 2021-01-06 at 11:12 +0800, Xu Guangxin wrote:
> > fixes #8857
> > 
> > If we do not clear the enc_ctrl, we will reuse previous frames' data like
> > FrameType.
> > ---
> >  libavcodec/qsvenc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> > index 2bd2a56227..94473c4eab 100644
> > --- a/libavcodec/qsvenc.c
> > +++ b/libavcodec/qsvenc.c
> > @@ -1249,6 +1249,8 @@ static void clear_unused_frames(QSVEncContext *q)
> >      while (cur) {
> >          if (cur->used && !cur->surface.Data.Locked) {
> >              free_encoder_ctrl_payloads(&cur->enc_ctrl);
> > +            //do not reuse enc_ctrl from previous frame
> > +            memset(&cur->enc_ctrl, 0, sizeof(cur->enc_ctrl));
> >              if (cur->frame->format == AV_PIX_FMT_QSV) {
> >                  av_frame_unref(cur->frame);
> >              }
> 
> LGTM

Could someone help to merge this patch ? 
https://github.com/Intel-Media-SDK/MediaSDK/issues/2776 was fixed by this patchtoo. 

Thanks
Haihao
James Almer Sept. 9, 2021, 3:32 p.m. UTC | #3 
On 1/6/2021 12:12 AM, Xu Guangxin wrote:
> fixes #8857
> 
> If we do not clear the enc_ctrl, we will reuse previous frames' data like FrameType.
> ---
>   libavcodec/qsvenc.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index 2bd2a56227..94473c4eab 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1249,6 +1249,8 @@ static void clear_unused_frames(QSVEncContext *q)
>       while (cur) {
>           if (cur->used && !cur->surface.Data.Locked) {
>               free_encoder_ctrl_payloads(&cur->enc_ctrl);
> +            //do not reuse enc_ctrl from previous frame
> +            memset(&cur->enc_ctrl, 0, sizeof(cur->enc_ctrl));

I assume cur->enc_ctrl.ExtParam is not set, right? Otherwise this memset 
could lead to leaks.

>               if (cur->frame->format == AV_PIX_FMT_QSV) {
>                   av_frame_unref(cur->frame);
>               }
>
Xiang, Haihao Sept. 9, 2021, 3:41 p.m. UTC | #4 
On Thu, 2021-09-09 at 12:32 -0300, James Almer wrote:
> On 1/6/2021 12:12 AM, Xu Guangxin wrote:
> > fixes #8857
> > 
> > If we do not clear the enc_ctrl, we will reuse previous frames' data like
> > FrameType.
> > ---
> >   libavcodec/qsvenc.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> > index 2bd2a56227..94473c4eab 100644
> > --- a/libavcodec/qsvenc.c
> > +++ b/libavcodec/qsvenc.c
> > @@ -1249,6 +1249,8 @@ static void clear_unused_frames(QSVEncContext *q)
> >       while (cur) {
> >           if (cur->used && !cur->surface.Data.Locked) {
> >               free_encoder_ctrl_payloads(&cur->enc_ctrl);
> > +            //do not reuse enc_ctrl from previous frame
> > +            memset(&cur->enc_ctrl, 0, sizeof(cur->enc_ctrl));
> 
> I assume cur->enc_ctrl.ExtParam is not set, right? Otherwise this memset 
> could lead to leaks.

Right, it is not set. 

> 
> >               if (cur->frame->format == AV_PIX_FMT_QSV) {
> >                   av_frame_unref(cur->frame);
> >               }
> > 
> 
> _______________________________________________
> 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. 9, 2021, 3:54 p.m. UTC | #5 
On 9/9/2021 12:41 PM, Xiang, Haihao wrote:
> On Thu, 2021-09-09 at 12:32 -0300, James Almer wrote:
>> On 1/6/2021 12:12 AM, Xu Guangxin wrote:
>>> fixes #8857
>>>
>>> If we do not clear the enc_ctrl, we will reuse previous frames' data like
>>> FrameType.
>>> ---
>>>    libavcodec/qsvenc.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
>>> index 2bd2a56227..94473c4eab 100644
>>> --- a/libavcodec/qsvenc.c
>>> +++ b/libavcodec/qsvenc.c
>>> @@ -1249,6 +1249,8 @@ static void clear_unused_frames(QSVEncContext *q)
>>>        while (cur) {
>>>            if (cur->used && !cur->surface.Data.Locked) {
>>>                free_encoder_ctrl_payloads(&cur->enc_ctrl);
>>> +            //do not reuse enc_ctrl from previous frame
>>> +            memset(&cur->enc_ctrl, 0, sizeof(cur->enc_ctrl));
>>
>> I assume cur->enc_ctrl.ExtParam is not set, right? Otherwise this memset
>> could lead to leaks.
> 
> Right, it is not set.

Applied then.
Xiang, Haihao Sept. 10, 2021, 1:11 a.m. UTC | #6 
On Thu, 2021-09-09 at 12:54 -0300, James Almer wrote:
> On 9/9/2021 12:41 PM, Xiang, Haihao wrote:
> > On Thu, 2021-09-09 at 12:32 -0300, James Almer wrote:
> > > On 1/6/2021 12:12 AM, Xu Guangxin wrote:
> > > > fixes #8857
> > > > 
> > > > If we do not clear the enc_ctrl, we will reuse previous frames' data
> > > > like
> > > > FrameType.
> > > > ---
> > > >    libavcodec/qsvenc.c | 2 ++
> > > >    1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> > > > index 2bd2a56227..94473c4eab 100644
> > > > --- a/libavcodec/qsvenc.c
> > > > +++ b/libavcodec/qsvenc.c
> > > > @@ -1249,6 +1249,8 @@ static void clear_unused_frames(QSVEncContext *q)
> > > >        while (cur) {
> > > >            if (cur->used && !cur->surface.Data.Locked) {
> > > >                free_encoder_ctrl_payloads(&cur->enc_ctrl);
> > > > +            //do not reuse enc_ctrl from previous frame
> > > > +            memset(&cur->enc_ctrl, 0, sizeof(cur->enc_ctrl));
> > > 
> > > I assume cur->enc_ctrl.ExtParam is not set, right? Otherwise this memset
> > > could lead to leaks.
> > 
> > Right, it is not set.
> 
> Applied then.

Thanks James
diff mbox series

Patch

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index 2bd2a56227..94473c4eab 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -1249,6 +1249,8 @@  static void clear_unused_frames(QSVEncContext *q)
     while (cur) {
         if (cur->used && !cur->surface.Data.Locked) {
             free_encoder_ctrl_payloads(&cur->enc_ctrl);
+            //do not reuse enc_ctrl from previous frame
+            memset(&cur->enc_ctrl, 0, sizeof(cur->enc_ctrl));
             if (cur->frame->format == AV_PIX_FMT_QSV) {
                 av_frame_unref(cur->frame);
             }