| Message ID | tencent_0367CAAAA6130CDC740BB06601108926D406@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/6] aarch64/hevc: Simplify function prototypes by macro | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting Zhao Zhili (2024-09-07 19:13:40) > From: Zhao Zhili <zhilizhao@tencent.com> > > --- > libavcodec/hevc/dsp_template.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/hevc/dsp_template.c b/libavcodec/hevc/dsp_template.c > index aebccd1a0c..a0f79c2673 100644 > --- a/libavcodec/hevc/dsp_template.c > +++ b/libavcodec/hevc/dsp_template.c > @@ -302,8 +302,8 @@ IDCT_DC(32) > //////////////////////////////////////////////////////////////////////////////// > #define ff_hevc_pel_filters ff_hevc_qpel_filters > #define DECL_HV_FILTER(f) \ > - const uint8_t *hf = ff_hevc_ ## f ## _filters[mx]; \ > - const uint8_t *vf = ff_hevc_ ## f ## _filters[my]; > + const int8_t *hf = ff_hevc_ ## f ## _filters[mx]; \ > + const int8_t *vf = ff_hevc_ ## f ## _filters[my]; Looks ok, though I wonder why are these then used as intptr_t.
> On Sep 9, 2024, at 18:35, Anton Khirnov <anton@khirnov.net> wrote: > > Quoting Zhao Zhili (2024-09-07 19:13:40) >> From: Zhao Zhili <zhilizhao@tencent.com> >> >> --- >> libavcodec/hevc/dsp_template.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/hevc/dsp_template.c b/libavcodec/hevc/dsp_template.c >> index aebccd1a0c..a0f79c2673 100644 >> --- a/libavcodec/hevc/dsp_template.c >> +++ b/libavcodec/hevc/dsp_template.c >> @@ -302,8 +302,8 @@ IDCT_DC(32) >> //////////////////////////////////////////////////////////////////////////////// >> #define ff_hevc_pel_filters ff_hevc_qpel_filters >> #define DECL_HV_FILTER(f) \ >> - const uint8_t *hf = ff_hevc_ ## f ## _filters[mx]; \ >> - const uint8_t *vf = ff_hevc_ ## f ## _filters[my]; >> + const int8_t *hf = ff_hevc_ ## f ## _filters[mx]; \ >> + const int8_t *vf = ff_hevc_ ## f ## _filters[my]; > > Looks ok, though I wonder why are these then used as intptr_t. They are accessed as const int8_t * in h256_inter_template.c. For example static void put_hevc_pel_pixels_8(int16_t *dst, const uint8_t *src, ptrdiff_t srcstride, int height, intptr_t mx, intptr_t my, int width) { const uint8_t *hf = ff_hevc_qpel_filters[mx]; const uint8_t *vf = ff_hevc_qpel_filters[my]; put_pixels_8(dst, src, srcstride, height, hf, vf, width); } I experienced a lot of headaches with these macro. > > -- > Anton Khirnov > _______________________________________________ > 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".
On Mon, Sep 9, 2024 at 6:35 PM Anton Khirnov <anton@khirnov.net> wrote: > Quoting Zhao Zhili (2024-09-07 19:13:40) > > From: Zhao Zhili <zhilizhao@tencent.com> > > > > --- > > libavcodec/hevc/dsp_template.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libavcodec/hevc/dsp_template.c > b/libavcodec/hevc/dsp_template.c > > index aebccd1a0c..a0f79c2673 100644 > > --- a/libavcodec/hevc/dsp_template.c > > +++ b/libavcodec/hevc/dsp_template.c > > @@ -302,8 +302,8 @@ IDCT_DC(32) > > > //////////////////////////////////////////////////////////////////////////////// > > #define ff_hevc_pel_filters ff_hevc_qpel_filters > > #define DECL_HV_FILTER(f) \ > > - const uint8_t *hf = ff_hevc_ ## f ## _filters[mx]; \ > > - const uint8_t *vf = ff_hevc_ ## f ## _filters[my]; > > + const int8_t *hf = ff_hevc_ ## f ## _filters[mx]; \ > > + const int8_t *vf = ff_hevc_ ## f ## _filters[my]; > > Looks ok, though I wonder why are these then used as intptr_t. > The original HEVC assembly code <https://github.com/FFmpeg/FFmpeg/blob/b3bbbb14d0685c8c1fbcf8455e59c7f444290c7c/libavcodec/x86/hevc_mc.asm#L204> uses mx and my as 64-bit variables. If we define it as an int, it will push a 32-bit value onto the stack, leaving the upper 32 bits undefined. Defining it as intptr_t will ensure all bits are filled. We no longer need to use intptr_t for x86 and NEON, as the assembly code now accepts const int8_t *.However, other platforms like MIPS still use intptr_t. Once all architectures are updated to use the const int8_t * style, we can modify HEVCDSPContext->put_hevc_xxx(intptr_t mx, intptr_t my...) to HEVCDSPContext->put_hevc_xxx(... const int8_t *hf, const int8_t *vf...). > -- > Anton Khirnov > _______________________________________________ > 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". >
On Mon, Sep 9, 2024 at 8:04 PM Zhao Zhili <quinkblack@foxmail.com> wrote: > > > > On Sep 9, 2024, at 18:35, Anton Khirnov <anton@khirnov.net> wrote: > > > > Quoting Zhao Zhili (2024-09-07 19:13:40) > >> From: Zhao Zhili <zhilizhao@tencent.com> > >> > >> --- > >> libavcodec/hevc/dsp_template.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/libavcodec/hevc/dsp_template.c > b/libavcodec/hevc/dsp_template.c > >> index aebccd1a0c..a0f79c2673 100644 > >> --- a/libavcodec/hevc/dsp_template.c > >> +++ b/libavcodec/hevc/dsp_template.c > >> @@ -302,8 +302,8 @@ IDCT_DC(32) > >> > //////////////////////////////////////////////////////////////////////////////// > >> #define ff_hevc_pel_filters ff_hevc_qpel_filters > >> #define DECL_HV_FILTER(f) \ > >> - const uint8_t *hf = ff_hevc_ ## f ## _filters[mx]; \ > >> - const uint8_t *vf = ff_hevc_ ## f ## _filters[my]; > >> + const int8_t *hf = ff_hevc_ ## f ## _filters[mx]; \ > >> + const int8_t *vf = ff_hevc_ ## f ## _filters[my]; > > > > Looks ok, though I wonder why are these then used as intptr_t. > > They are accessed as const int8_t * in h256_inter_template.c. For example > > static void > put_hevc_pel_pixels_8(int16_t *dst, const uint8_t *src, ptrdiff_t > srcstride, > int height, intptr_t mx, intptr_t my, int width) { > const uint8_t *hf = ff_hevc_qpel_filters[mx]; > const uint8_t *vf = ff_hevc_qpel_filters[my]; > put_pixels_8(dst, src, srcstride, height, hf, vf, width); > } > > I experienced a lot of headaches with these macro. > Yeah. Difficult to find, difficult to read, and difficult to understand. > > > > > -- > > Anton Khirnov > > _______________________________________________ > > 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". >
On Sun, 8 Sep 2024, Zhao Zhili wrote: > From: Zhao Zhili <zhilizhao@tencent.com> > > --- > libavcodec/hevc/dsp_template.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) The rest of these patches seem fine (or where I don't have much of an opinion). // Martin
diff --git a/libavcodec/hevc/dsp_template.c b/libavcodec/hevc/dsp_template.c index aebccd1a0c..a0f79c2673 100644 --- a/libavcodec/hevc/dsp_template.c +++ b/libavcodec/hevc/dsp_template.c @@ -302,8 +302,8 @@ IDCT_DC(32) //////////////////////////////////////////////////////////////////////////////// #define ff_hevc_pel_filters ff_hevc_qpel_filters #define DECL_HV_FILTER(f) \ - const uint8_t *hf = ff_hevc_ ## f ## _filters[mx]; \ - const uint8_t *vf = ff_hevc_ ## f ## _filters[my]; + const int8_t *hf = ff_hevc_ ## f ## _filters[mx]; \ + const int8_t *vf = ff_hevc_ ## f ## _filters[my]; #define FW_PUT(p, f, t) \ static void FUNC(put_hevc_## f)(int16_t *dst, const uint8_t *src, ptrdiff_t srcstride, int height, \
From: Zhao Zhili <zhilizhao@tencent.com> --- libavcodec/hevc/dsp_template.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)