[FFmpeg-devel] [PATCH] lavc/vp9: set update_map to 0 when segmentation.enabled is 0

[FFmpeg-devel] [PATCH] lavc/vp9: set update_map to 0 when segmentation.enabled is 0

[llyyr]

segmentation.update_map is never reset to 0 on a new frame, and retains
the value from the previous frame. This bugs out a bunch of hwaccel
drivers when segmentation.enabled is 0 but update_map isn't because
they don't ignore values behind switches. We also do this for vp8* so
this commit is just mirroring the vp8 logic.

This fixes an issue with certain samples** that causes blocky
artifacts with vaapi and d3d11va (as far as known hwaccel drivers go).
Mesa worked around*** this by ignoring this field if
segmentation.enabled is 0, but d3d11va still doesn't work.

* https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/HEAD:/libavcodec/vp8.c#l811
** https://github.com/mpv-player/mpv/issues/13533
*** https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816

Signed-off-by: llyyr <llyyr@yukari.in>
---
 libavcodec/vp9.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
index 855936cdc1c7..4a628625131e 100644
--- a/libavcodec/vp9.c
+++ b/libavcodec/vp9.c
@@ -717,6 +717,8 @@ static int decode_frame_header(AVCodecContext *avctx,
                 s->s.h.segmentation.feat[i].skip_enabled = get_bits1(&s->gb);
             }
         }
+    } else {
+        s->s.h.segmentation.update_map = 0;
     }
 
     // set qmul[] based on Y/UV, AC/DC and segmentation Q idx deltas

base-commit: d263fce2b209e86a5a1e8f1b6aa33430ecc2c187
-- 
2.43.2
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] lavc/vp9: set update_map to 0 when segmentation.enabled is 0

[Hendrik Leppkes]

On Thu, Feb 29, 2024 at 7:19 AM llyyr <llyyr@yukari.in> wrote:
>
> segmentation.update_map is never reset to 0 on a new frame, and retains
> the value from the previous frame. This bugs out a bunch of hwaccel
> drivers when segmentation.enabled is 0 but update_map isn't because
> they don't ignore values behind switches. We also do this for vp8* so
> this commit is just mirroring the vp8 logic.
>
> This fixes an issue with certain samples** that causes blocky
> artifacts with vaapi and d3d11va (as far as known hwaccel drivers go).
> Mesa worked around*** this by ignoring this field if
> segmentation.enabled is 0, but d3d11va still doesn't work.
>
> * https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/HEAD:/libavcodec/vp8.c#l811
> ** https://github.com/mpv-player/mpv/issues/13533
> *** https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816
>
> Signed-off-by: llyyr <llyyr@yukari.in>
> ---
>  libavcodec/vp9.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> index 855936cdc1c7..4a628625131e 100644
> --- a/libavcodec/vp9.c
> +++ b/libavcodec/vp9.c
> @@ -717,6 +717,8 @@ static int decode_frame_header(AVCodecContext *avctx,
>                  s->s.h.segmentation.feat[i].skip_enabled = get_bits1(&s->gb);
>              }
>          }
> +    } else {
> +        s->s.h.segmentation.update_map = 0;
>      }
>
>      // set qmul[] based on Y/UV, AC/DC and segmentation Q idx deltas
>
> base-commit: d263fce2b209e86a5a1e8f1b6aa33430ecc2c187
> --

Change LGTM.
I was debugging the same issue today, and found the same problem with
some hwaccels not properly ignoring update_map when segmentation is
disabled.

Will apply soon if there are no further comments.

- Hendrik
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] lavc/vp9: set update_map to 0 when segmentation.enabled is 0

[Ronald S. Bultje]

Hi,

On Wed, May 22, 2024 at 10:36 AM Hendrik Leppkes <h.leppkes@gmail.com>
wrote:

> On Thu, Feb 29, 2024 at 7:19 AM llyyr <llyyr@yukari.in> wrote:
> >
> > segmentation.update_map is never reset to 0 on a new frame, and retains
> > the value from the previous frame. This bugs out a bunch of hwaccel
> > drivers when segmentation.enabled is 0 but update_map isn't because
> > they don't ignore values behind switches. We also do this for vp8* so
> > this commit is just mirroring the vp8 logic.
> >
> > This fixes an issue with certain samples** that causes blocky
> > artifacts with vaapi and d3d11va (as far as known hwaccel drivers go).
> > Mesa worked around*** this by ignoring this field if
> > segmentation.enabled is 0, but d3d11va still doesn't work.
> >
> > *
> https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/HEAD:/libavcodec/vp8.c#l811
> > ** https://github.com/mpv-player/mpv/issues/13533
> > *** https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816
> >
> > Signed-off-by: llyyr <llyyr@yukari.in>
> > ---
> >  libavcodec/vp9.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> > index 855936cdc1c7..4a628625131e 100644
> > --- a/libavcodec/vp9.c
> > +++ b/libavcodec/vp9.c
> > @@ -717,6 +717,8 @@ static int decode_frame_header(AVCodecContext *avctx,
> >                  s->s.h.segmentation.feat[i].skip_enabled =
> get_bits1(&s->gb);
> >              }
> >          }
> > +    } else {
> > +        s->s.h.segmentation.update_map = 0;
> >      }
> >
> >      // set qmul[] based on Y/UV, AC/DC and segmentation Q idx deltas
> >
> > base-commit: d263fce2b209e86a5a1e8f1b6aa33430ecc2c187
> > --
>
> Change LGTM.
> I was debugging the same issue today, and found the same problem with
> some hwaccels not properly ignoring update_map when segmentation is
> disabled.
>
> Will apply soon if there are no further comments.
>

Is fine, please apply.

Ronald
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] lavc/vp9: set update_map to 0 when segmentation.enabled is 0

[Philip Langdale via ffmpeg-devel]

On Wed, 22 May 2024 11:10:31 -0400
"Ronald S. Bultje" <rsbultje@gmail.com> wrote:

> Hi,
> 
> On Wed, May 22, 2024 at 10:36 AM Hendrik Leppkes <h.leppkes@gmail.com>
> wrote:
> 
> > On Thu, Feb 29, 2024 at 7:19 AM llyyr <llyyr@yukari.in> wrote:  
> > >
> > > segmentation.update_map is never reset to 0 on a new frame, and
> > > retains the value from the previous frame. This bugs out a bunch
> > > of hwaccel drivers when segmentation.enabled is 0 but update_map
> > > isn't because they don't ignore values behind switches. We also
> > > do this for vp8* so this commit is just mirroring the vp8 logic.
> > >
> > > This fixes an issue with certain samples** that causes blocky
> > > artifacts with vaapi and d3d11va (as far as known hwaccel drivers
> > > go). Mesa worked around*** this by ignoring this field if
> > > segmentation.enabled is 0, but d3d11va still doesn't work.
> > >
> > > *  
> > https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/HEAD:/libavcodec/vp8.c#l811
> >  
> > > ** https://github.com/mpv-player/mpv/issues/13533
> > > ***
> > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816
> > >
> > > Signed-off-by: llyyr <llyyr@yukari.in>
> > > ---
> > >  libavcodec/vp9.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> > > index 855936cdc1c7..4a628625131e 100644
> > > --- a/libavcodec/vp9.c
> > > +++ b/libavcodec/vp9.c
> > > @@ -717,6 +717,8 @@ static int decode_frame_header(AVCodecContext
> > > *avctx, s->s.h.segmentation.feat[i].skip_enabled =  
> > get_bits1(&s->gb);  
> > >              }
> > >          }
> > > +    } else {
> > > +        s->s.h.segmentation.update_map = 0;
> > >      }
> > >
> > >      // set qmul[] based on Y/UV, AC/DC and segmentation Q idx
> > > deltas
> > >
> > > base-commit: d263fce2b209e86a5a1e8f1b6aa33430ecc2c187
> > > --  
> >
> > Change LGTM.
> > I was debugging the same issue today, and found the same problem
> > with some hwaccels not properly ignoring update_map when
> > segmentation is disabled.
> >
> > Will apply soon if there are no further comments.
> >  
> 
> Is fine, please apply.
> 

Another LGTM. We've been seeing this reported on the mpv side as well.

--phil
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] lavc/vp9: set update_map to 0 when segmentation.enabled is 0

[Lynne via ffmpeg-devel]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2783 bytes --]

On 22/05/2024 18:17, Philip Langdale via ffmpeg-devel wrote:
> On Wed, 22 May 2024 11:10:31 -0400
> "Ronald S. Bultje" <rsbultje@gmail.com> wrote:
> 
>> Hi,
>>
>> On Wed, May 22, 2024 at 10:36 AM Hendrik Leppkes <h.leppkes@gmail.com>
>> wrote:
>>
>>> On Thu, Feb 29, 2024 at 7:19 AM llyyr <llyyr@yukari.in> wrote:
>>>>
>>>> segmentation.update_map is never reset to 0 on a new frame, and
>>>> retains the value from the previous frame. This bugs out a bunch
>>>> of hwaccel drivers when segmentation.enabled is 0 but update_map
>>>> isn't because they don't ignore values behind switches. We also
>>>> do this for vp8* so this commit is just mirroring the vp8 logic.
>>>>
>>>> This fixes an issue with certain samples** that causes blocky
>>>> artifacts with vaapi and d3d11va (as far as known hwaccel drivers
>>>> go). Mesa worked around*** this by ignoring this field if
>>>> segmentation.enabled is 0, but d3d11va still doesn't work.
>>>>
>>>> *
>>> https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/HEAD:/libavcodec/vp8.c#l811
>>>   
>>>> ** https://github.com/mpv-player/mpv/issues/13533
>>>> ***
>>>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816
>>>>
>>>> Signed-off-by: llyyr <llyyr@yukari.in>
>>>> ---
>>>>   libavcodec/vp9.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
>>>> index 855936cdc1c7..4a628625131e 100644
>>>> --- a/libavcodec/vp9.c
>>>> +++ b/libavcodec/vp9.c
>>>> @@ -717,6 +717,8 @@ static int decode_frame_header(AVCodecContext
>>>> *avctx, s->s.h.segmentation.feat[i].skip_enabled =
>>> get_bits1(&s->gb);
>>>>               }
>>>>           }
>>>> +    } else {
>>>> +        s->s.h.segmentation.update_map = 0;
>>>>       }
>>>>
>>>>       // set qmul[] based on Y/UV, AC/DC and segmentation Q idx
>>>> deltas
>>>>
>>>> base-commit: d263fce2b209e86a5a1e8f1b6aa33430ecc2c187
>>>> --
>>>
>>> Change LGTM.
>>> I was debugging the same issue today, and found the same problem
>>> with some hwaccels not properly ignoring update_map when
>>> segmentation is disabled.
>>>
>>> Will apply soon if there are no further comments.
>>>   
>>
>> Is fine, please apply.
>>
> 
> Another LGTM. We've been seeing this reported on the mpv side as well.
> 
> --phil
> _______________________________________________
> 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".

Can't this get fixed by hwaccel code rather than globally?
I'd hate to apply fixes with no information in shared code. This can get 
removed with no information about what relies on it.

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 637 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] lavc/vp9: set update_map to 0 when segmentation.enabled is 0

[Hendrik Leppkes]

On Wed, May 22, 2024 at 7:16 PM Lynne via ffmpeg-devel
<ffmpeg-devel@ffmpeg.org> wrote:
>
> On 22/05/2024 18:17, Philip Langdale via ffmpeg-devel wrote:
> > On Wed, 22 May 2024 11:10:31 -0400
> > "Ronald S. Bultje" <rsbultje@gmail.com> wrote:
> >
> >> Hi,
> >>
> >> On Wed, May 22, 2024 at 10:36 AM Hendrik Leppkes <h.leppkes@gmail.com>
> >> wrote:
> >>
> >>> On Thu, Feb 29, 2024 at 7:19 AM llyyr <llyyr@yukari.in> wrote:
> >>>>
> >>>> segmentation.update_map is never reset to 0 on a new frame, and
> >>>> retains the value from the previous frame. This bugs out a bunch
> >>>> of hwaccel drivers when segmentation.enabled is 0 but update_map
> >>>> isn't because they don't ignore values behind switches. We also
> >>>> do this for vp8* so this commit is just mirroring the vp8 logic.
> >>>>
> >>>> This fixes an issue with certain samples** that causes blocky
> >>>> artifacts with vaapi and d3d11va (as far as known hwaccel drivers
> >>>> go). Mesa worked around*** this by ignoring this field if
> >>>> segmentation.enabled is 0, but d3d11va still doesn't work.
> >>>>
> >>>> *
> >>> https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/HEAD:/libavcodec/vp8.c#l811
> >>>
> >>>> ** https://github.com/mpv-player/mpv/issues/13533
> >>>> ***
> >>>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816
> >>>>
> >>>> Signed-off-by: llyyr <llyyr@yukari.in>
> >>>> ---
> >>>>   libavcodec/vp9.c | 2 ++
> >>>>   1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> >>>> index 855936cdc1c7..4a628625131e 100644
> >>>> --- a/libavcodec/vp9.c
> >>>> +++ b/libavcodec/vp9.c
> >>>> @@ -717,6 +717,8 @@ static int decode_frame_header(AVCodecContext
> >>>> *avctx, s->s.h.segmentation.feat[i].skip_enabled =
> >>> get_bits1(&s->gb);
> >>>>               }
> >>>>           }
> >>>> +    } else {
> >>>> +        s->s.h.segmentation.update_map = 0;
> >>>>       }
> >>>>
> >>>>       // set qmul[] based on Y/UV, AC/DC and segmentation Q idx
> >>>> deltas
> >>>>
> >>>> base-commit: d263fce2b209e86a5a1e8f1b6aa33430ecc2c187
> >>>> --
> >>>
> >>> Change LGTM.
> >>> I was debugging the same issue today, and found the same problem
> >>> with some hwaccels not properly ignoring update_map when
> >>> segmentation is disabled.
> >>>
> >>> Will apply soon if there are no further comments.
> >>>
> >>
> >> Is fine, please apply.
> >>
> >
> > Another LGTM. We've been seeing this reported on the mpv side as well.
> >
> > --phil
> > _______________________________________________
> > 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".
>
> Can't this get fixed by hwaccel code rather than globally?
> I'd hate to apply fixes with no information in shared code. This can get
> removed with no information about what relies on it.

Changing 5 different hwaccel modules to avoid one line here seems
rather silly, doesn't it?
We can add a comment, if that helps.

- Hendrik
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] lavc/vp9: set update_map to 0 when segmentation.enabled is 0

[Ronald S. Bultje]

Hi,

On Wed, May 22, 2024 at 1:28 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:

> On Wed, May 22, 2024 at 7:16 PM Lynne via ffmpeg-devel
> <ffmpeg-devel@ffmpeg.org> wrote:
> > I'd hate to apply fixes with no information in shared code. This can get
> > removed with no information about what relies on it.
>
> Changing 5 different hwaccel modules to avoid one line here seems
> rather silly, doesn't it?
> We can add a comment, if that helps.
>

Comment is a good idea, I think Lynne is right there's a risk we
accidentally remove it.

Generally, I'd like to see more hwaccel fate coverage, e.g. a vaapi (or
whatever) fate machine that runs relevant tests using hw decoder instead of
sw decoder. That would protect against the same risk.

Ronald
_______________________________________________
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".