diff mbox series

[FFmpeg-devel] avcodec/dxva2_av1: use correct grain parameters with update_grain = 0

Message ID 20201123113805.386-1-h.leppkes@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/dxva2_av1: use correct grain parameters with update_grain = 0
Related show

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

Hendrik Leppkes Nov. 23, 2020, 11:38 a.m. UTC
When update_grain is zero, the parameters should be taken from a
reference frame instead.
---
 libavcodec/dxva2_av1.c | 59 +++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

Comments

James Almer Nov. 23, 2020, 8:42 p.m. UTC | #1
On 11/23/2020 8:38 AM, Hendrik Leppkes wrote:
> When update_grain is zero, the parameters should be taken from a
> reference frame instead.
> ---
>   libavcodec/dxva2_av1.c | 59 +++++++++++++++++++++---------------------
>   1 file changed, 30 insertions(+), 29 deletions(-)

LGTM
Guangxin Xu Nov. 24, 2020, 8:59 a.m. UTC | #2
Hi   Hendrik,
two related questions:
1. Seems it's a generic code. Could we handle this in the av1dec.c?
2. For the current code, what's happened if frame A ref to B, B ref to C.
and both A, B has update_grain == 0.
thanks


On Tue, Nov 24, 2020 at 4:42 AM James Almer <jamrial@gmail.com> wrote:

> On 11/23/2020 8:38 AM, Hendrik Leppkes wrote:
> > When update_grain is zero, the parameters should be taken from a
> > reference frame instead.
> > ---
> >   libavcodec/dxva2_av1.c | 59 +++++++++++++++++++++---------------------
> >   1 file changed, 30 insertions(+), 29 deletions(-)
>
> LGTM
> _______________________________________________
> 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 Nov. 25, 2020, 7:05 p.m. UTC | #3
On 11/24/2020 5:59 AM, Guangxin Xu wrote:
> Hi   Hendrik,
> two related questions:
> 1. Seems it's a generic code. Could we handle this in the av1dec.c?
> 2. For the current code, what's happened if frame A ref to B, B ref to C.
> and both A, B has update_grain == 0.

Is that considered a valid bitstream? If so, then value inferring should 
be done in either CBS (like it's done for segmentation and loop filter), 
or in av1dec.c. Otherwise all the fields will be zero, same as is the 
case right now.

> thanks
> 
> 
> On Tue, Nov 24, 2020 at 4:42 AM James Almer <jamrial@gmail.com> wrote:
> 
>> On 11/23/2020 8:38 AM, Hendrik Leppkes wrote:
>>> When update_grain is zero, the parameters should be taken from a
>>> reference frame instead.
>>> ---
>>>    libavcodec/dxva2_av1.c | 59 +++++++++++++++++++++---------------------
>>>    1 file changed, 30 insertions(+), 29 deletions(-)
>>
>> LGTM
>> _______________________________________________
>> 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 Nov. 26, 2020, 5:34 a.m. UTC | #4
spec did not forbid this.
Here is da1vd code:
https://code.videolan.org/videolan/dav1d/-/blob/master/src/obu.c#L1057
If  update_grain  == 0, it will copy film grain data from the reference
frame to the current frame.
Maybe we can do the same thing on CBS, Other acceleration like vaapi,
will get benefit from this.


On Thu, Nov 26, 2020 at 3:05 AM James Almer <jamrial@gmail.com> wrote:

> On 11/24/2020 5:59 AM, Guangxin Xu wrote:
> > Hi   Hendrik,
> > two related questions:
> > 1. Seems it's a generic code. Could we handle this in the av1dec.c?
> > 2. For the current code, what's happened if frame A ref to B, B ref to C.
> > and both A, B has update_grain == 0.
>
> Is that considered a valid bitstream? If so, then value inferring should
> be done in either CBS (like it's done for segmentation and loop filter),
> or in av1dec.c. Otherwise all the fields will be zero, same as is the
> case right now.
>
> > thanks
> >
> >
> > On Tue, Nov 24, 2020 at 4:42 AM James Almer <jamrial@gmail.com> wrote:
> >
> >> On 11/23/2020 8:38 AM, Hendrik Leppkes wrote:
> >>> When update_grain is zero, the parameters should be taken from a
> >>> reference frame instead.
> >>> ---
> >>>    libavcodec/dxva2_av1.c | 59
> +++++++++++++++++++++---------------------
> >>>    1 file changed, 30 insertions(+), 29 deletions(-)
> >>
> >> LGTM
> >> _______________________________________________
> >> 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".
James Almer Nov. 26, 2020, 1:05 p.m. UTC | #5
On 11/26/2020 2:34 AM, Guangxin Xu wrote:
> spec did not forbid this.
> Here is da1vd code:
> https://code.videolan.org/videolan/dav1d/-/blob/master/src/obu.c#L1057
> If  update_grain  == 0, it will copy film grain data from the reference
> frame to the current frame.
> Maybe we can do the same thing on CBS, Other acceleration like vaapi,
> will get benefit from this.

Yes, I sent a patchset to properly copy film grain params from reference 
frames yesterday that should cover all possible scenarios.

Regarding Vaapi, its film grain implementation is incomplete, on top of 
also being affected by this issue. It doesn't look like it's filling all 
the VAFilmGrainStructAV1 fields it should.

> 
> 
> On Thu, Nov 26, 2020 at 3:05 AM James Almer <jamrial@gmail.com> wrote:
> 
>> On 11/24/2020 5:59 AM, Guangxin Xu wrote:
>>> Hi   Hendrik,
>>> two related questions:
>>> 1. Seems it's a generic code. Could we handle this in the av1dec.c?
>>> 2. For the current code, what's happened if frame A ref to B, B ref to C.
>>> and both A, B has update_grain == 0.
>>
>> Is that considered a valid bitstream? If so, then value inferring should
>> be done in either CBS (like it's done for segmentation and loop filter),
>> or in av1dec.c. Otherwise all the fields will be zero, same as is the
>> case right now.
>>
>>> thanks
>>>
>>>
>>> On Tue, Nov 24, 2020 at 4:42 AM James Almer <jamrial@gmail.com> wrote:
>>>
>>>> On 11/23/2020 8:38 AM, Hendrik Leppkes wrote:
>>>>> When update_grain is zero, the parameters should be taken from a
>>>>> reference frame instead.
>>>>> ---
>>>>>     libavcodec/dxva2_av1.c | 59
>> +++++++++++++++++++++---------------------
>>>>>     1 file changed, 30 insertions(+), 29 deletions(-)
>>>>
>>>> LGTM
>>>> _______________________________________________
>>>> 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".
> _______________________________________________
> 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/dxva2_av1.c b/libavcodec/dxva2_av1.c
index c6cbd48f5f..35d0bdf319 100644
--- a/libavcodec/dxva2_av1.c
+++ b/libavcodec/dxva2_av1.c
@@ -215,46 +215,47 @@  static int fill_picture_parameters(const AVCodecContext *avctx, AVDXVAContext *c
 
     /* Film grain */
     if (frame_header->apply_grain) {
+        const AV1RawFrameHeader *fg_header = frame_header->update_grain ? frame_header : h->ref[frame_header->film_grain_params_ref_idx].raw_frame_header;
         pp->film_grain.apply_grain              = 1;
-        pp->film_grain.scaling_shift_minus8     = frame_header->grain_scaling_minus_8;
-        pp->film_grain.chroma_scaling_from_luma = frame_header->chroma_scaling_from_luma;
-        pp->film_grain.ar_coeff_lag             = frame_header->ar_coeff_lag;
-        pp->film_grain.ar_coeff_shift_minus6    = frame_header->ar_coeff_shift_minus_6;
-        pp->film_grain.grain_scale_shift        = frame_header->grain_scale_shift;
-        pp->film_grain.overlap_flag             = frame_header->overlap_flag;
-        pp->film_grain.clip_to_restricted_range = frame_header->clip_to_restricted_range;
+        pp->film_grain.scaling_shift_minus8     = fg_header->grain_scaling_minus_8;
+        pp->film_grain.chroma_scaling_from_luma = fg_header->chroma_scaling_from_luma;
+        pp->film_grain.ar_coeff_lag             = fg_header->ar_coeff_lag;
+        pp->film_grain.ar_coeff_shift_minus6    = fg_header->ar_coeff_shift_minus_6;
+        pp->film_grain.grain_scale_shift        = fg_header->grain_scale_shift;
+        pp->film_grain.overlap_flag             = fg_header->overlap_flag;
+        pp->film_grain.clip_to_restricted_range = fg_header->clip_to_restricted_range;
         pp->film_grain.matrix_coeff_is_identity = (seq->color_config.matrix_coefficients == AVCOL_SPC_RGB);
 
         pp->film_grain.grain_seed               = frame_header->grain_seed;
-        pp->film_grain.num_y_points             = frame_header->num_y_points;
-        for (i = 0; i < frame_header->num_y_points; i++) {
-            pp->film_grain.scaling_points_y[i][0] = frame_header->point_y_value[i];
-            pp->film_grain.scaling_points_y[i][1] = frame_header->point_y_scaling[i];
+        pp->film_grain.num_y_points             = fg_header->num_y_points;
+        for (i = 0; i < fg_header->num_y_points; i++) {
+            pp->film_grain.scaling_points_y[i][0] = fg_header->point_y_value[i];
+            pp->film_grain.scaling_points_y[i][1] = fg_header->point_y_scaling[i];
         }
-        pp->film_grain.num_cb_points            = frame_header->num_cb_points;
-        for (i = 0; i < frame_header->num_cb_points; i++) {
-            pp->film_grain.scaling_points_cb[i][0] = frame_header->point_cb_value[i];
-            pp->film_grain.scaling_points_cb[i][1] = frame_header->point_cb_scaling[i];
+        pp->film_grain.num_cb_points            = fg_header->num_cb_points;
+        for (i = 0; i < fg_header->num_cb_points; i++) {
+            pp->film_grain.scaling_points_cb[i][0] = fg_header->point_cb_value[i];
+            pp->film_grain.scaling_points_cb[i][1] = fg_header->point_cb_scaling[i];
         }
-        pp->film_grain.num_cr_points            = frame_header->num_cr_points;
-        for (i = 0; i < frame_header->num_cr_points; i++) {
-            pp->film_grain.scaling_points_cr[i][0] = frame_header->point_cr_value[i];
-            pp->film_grain.scaling_points_cr[i][1] = frame_header->point_cr_scaling[i];
+        pp->film_grain.num_cr_points            = fg_header->num_cr_points;
+        for (i = 0; i < fg_header->num_cr_points; i++) {
+            pp->film_grain.scaling_points_cr[i][0] = fg_header->point_cr_value[i];
+            pp->film_grain.scaling_points_cr[i][1] = fg_header->point_cr_scaling[i];
         }
         for (i = 0; i < 24; i++) {
-            pp->film_grain.ar_coeffs_y[i] = frame_header->ar_coeffs_y_plus_128[i];
+            pp->film_grain.ar_coeffs_y[i] = fg_header->ar_coeffs_y_plus_128[i];
         }
         for (i = 0; i < 25; i++) {
-            pp->film_grain.ar_coeffs_cb[i] = frame_header->ar_coeffs_cb_plus_128[i];
-            pp->film_grain.ar_coeffs_cr[i] = frame_header->ar_coeffs_cr_plus_128[i];
+            pp->film_grain.ar_coeffs_cb[i] = fg_header->ar_coeffs_cb_plus_128[i];
+            pp->film_grain.ar_coeffs_cr[i] = fg_header->ar_coeffs_cr_plus_128[i];
         }
-        pp->film_grain.cb_mult      = frame_header->cb_mult;
-        pp->film_grain.cb_luma_mult = frame_header->cb_luma_mult;
-        pp->film_grain.cr_mult      = frame_header->cr_mult;
-        pp->film_grain.cr_luma_mult = frame_header->cr_luma_mult;
-        pp->film_grain.cb_offset    = frame_header->cb_offset;
-        pp->film_grain.cr_offset    = frame_header->cr_offset;
-        pp->film_grain.cr_offset    = frame_header->cr_offset;
+        pp->film_grain.cb_mult      = fg_header->cb_mult;
+        pp->film_grain.cb_luma_mult = fg_header->cb_luma_mult;
+        pp->film_grain.cr_mult      = fg_header->cr_mult;
+        pp->film_grain.cr_luma_mult = fg_header->cr_luma_mult;
+        pp->film_grain.cb_offset    = fg_header->cb_offset;
+        pp->film_grain.cr_offset    = fg_header->cr_offset;
+        pp->film_grain.cr_offset    = fg_header->cr_offset;
     }
 
     // XXX: Setting the StatusReportFeedbackNumber breaks decoding on some drivers (tested on NVIDIA 457.09)