| Message ID | 20201102190551.1223389-1-adrian.ratiu@collabora.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: rkvdec: Add a VP9 backend | expand |
On Mon, 2 Nov 2020 at 16:04, Adrian Ratiu <adrian.ratiu@collabora.com> wrote: > > Dear all, > > This is v5 of the series adding VP9 profile 0 decoding to rkvdec. > > All feedback from v4 should be addressed, there's just one thing I did > not address: ref_frame_sign_biases in the uAPI. The userspace tool I'm I believe that Hantro G2 VP9 needs ref_frame_sign_biases. I think that it's also needed for the MTK decoder. Might be worth checking that as well, if the code is publicly available somewhere. Coming to think about it, I think we are really close to having this uAPI directly upstream. Let's take a step back on why we have these uAPIs in the staging area. Couple years ago, there were some doubts in the media community about these uAPIs, and we wanted to wait a bit for more users before moving to public land. The uAPIs were meant to be in staging until enough users appeared and we were confident enough to move to stable. For VP9, given the feedback received through the year was already addressed, I think all that's left is to check the interface and make sure it can support Rockchip (RK3399, RK3326, etc), Hantro G2 and Mediatek, We will be very close to having a public API, and we could even merge it directly there. Thanks, Ezequiel > using [1] apparently doesn't need it or the default hwreg value for it > is capable of decoding the bitstreams I used on the driver, so I don't > really have a use-case to change and test that. :) > > Considering the uAPI is a work in progress and expected to be modified, > ref_frame_sign_biases can be added later with others which might be > required to enable more functionality (for eg profiles >= 1). > > Series tested on rk3399 and applies on next-20201030. > > [1] https://github.com/Kwiboo/FFmpeg/tree/v4l2-request-hwaccel-4.2.2-rkvdec > > Changelog > --------- > > v5: > > * Drop unnecessary OUTPUT buffer payload set in .buf_prepare. > * Drop obsolete .per_request ctrl flag > * Added new vp9 ctrls to v4l2_ctrl_ptr > * Fix pahole detected padding issues > * Send userspace an error if it tries to reconfigure decode resolution > as v4l2 or rkvdec-vp9 backend do not support dynamic res changes yet > * Allow frame ctx probability tables to be non-mandatory so users can > set them directly during frame decoding in cases where no defaults > have been set previously (eg. ffmpeg vp9 backend) > * Some comments and documentation clarifications > * Minor checkpatch fixes > > v4: > > * Drop color_space field from the VP9 interface. > V4L2 API should be used for it. > * Clarified Segment-ID comments. > * Moved motion vector probabilities to a separate > struct. > > v3: > > * Fix documentation issues found by Hans. > * Fix smatch detected issues as pointed out by Hans. > * Added patch to fix wrong bytesused set on .buf_prepare. > > v2: > > * Documentation style issues pointed out by Nicolas internally. > * s/VP9_PROFILE_MAX/V4L2_VP9_PROFILE_MAX/ > * Fix wrong kfree(ctx). > * constify a couple structs on rkvdec-vp9.c > > > Boris Brezillon (2): > media: uapi: Add VP9 stateless decoder controls > media: rkvdec: Add the VP9 backend > > Ezequiel Garcia (1): > media: rkvdec: Fix .buf_prepare > > .../userspace-api/media/v4l/biblio.rst | 10 + > .../media/v4l/ext-ctrls-codec.rst | 550 ++++++ > drivers/media/v4l2-core/v4l2-ctrls.c | 239 +++ > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + > drivers/staging/media/rkvdec/Makefile | 2 +- > drivers/staging/media/rkvdec/rkvdec-vp9.c | 1577 +++++++++++++++++ > drivers/staging/media/rkvdec/rkvdec.c | 72 +- > drivers/staging/media/rkvdec/rkvdec.h | 6 + > include/media/v4l2-ctrls.h | 5 + > include/media/vp9-ctrls.h | 486 +++++ > 10 files changed, 2942 insertions(+), 6 deletions(-) > create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c > create mode 100644 include/media/vp9-ctrls.h > > -- > 2.29.0 >
Hi Ezequiel, On Tue, 10 Nov 2020, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote: > On Mon, 2 Nov 2020 at 16:04, Adrian Ratiu > <adrian.ratiu@collabora.com> wrote: >> >> Dear all, >> >> This is v5 of the series adding VP9 profile 0 decoding to >> rkvdec. >> >> All feedback from v4 should be addressed, there's just one >> thing I did not address: ref_frame_sign_biases in the uAPI. The >> userspace tool I'm > > I believe that Hantro G2 VP9 needs ref_frame_sign_biases. > > I think that it's also needed for the MTK decoder. Might be > worth checking that as well, if the code is publicly available > somewhere. I consulted the imx8m app ref manual for the Hantro G2 core and indeed there's not one, but three fields at SWREG11 and 13 (last, gold, alt) to signify sign biases for ref frames. Thanks for the hint! > > Coming to think about it, I think we are really close to having > this uAPI directly upstream. > > Let's take a step back on why we have these uAPIs in the staging > area. Couple years ago, there were some doubts in the media > community about these uAPIs, and we wanted to wait a bit for > more users before moving to public land. > > The uAPIs were meant to be in staging until enough users > appeared and we were confident enough to move to stable. > > For VP9, given the feedback received through the year was > already addressed, I think all that's left is to check the > interface and make sure it can support Rockchip (RK3399, RK3326, > etc), Hantro G2 and Mediatek, > > We will be very close to having a public API, and we could even > merge it directly there. Thank you very much for this background. I understand that the uAPI is independent from the driver implementations, so having a good stable uAPI is beneficial when (for example) adding support for VP9 on G2 in hantro or for upstream adoption of these drivers. Given this rkvdec driver implementation is also adding the VP9 uAPI and it's very close to stability (maybe only missing ref frame sign bias, but who knows?) would you like to block its submission until the uAPI is finalized or would it make sense to treat the uAPI de-staging process separately because the uAPI is independent from the driver? Thanks, Adrian > > Thanks, > Ezequiel > >> using [1] apparently doesn't need it or the default hwreg value for it >> is capable of decoding the bitstreams I used on the driver, so I don't >> really have a use-case to change and test that. :) >> >> Considering the uAPI is a work in progress and expected to be modified, >> ref_frame_sign_biases can be added later with others which might be >> required to enable more functionality (for eg profiles >= 1). >> >> Series tested on rk3399 and applies on next-20201030. >> >> [1] https://github.com/Kwiboo/FFmpeg/tree/v4l2-request-hwaccel-4.2.2-rkvdec >> >> Changelog >> --------- >> >> v5: >> >> * Drop unnecessary OUTPUT buffer payload set in .buf_prepare. >> * Drop obsolete .per_request ctrl flag >> * Added new vp9 ctrls to v4l2_ctrl_ptr >> * Fix pahole detected padding issues >> * Send userspace an error if it tries to reconfigure decode resolution >> as v4l2 or rkvdec-vp9 backend do not support dynamic res changes yet >> * Allow frame ctx probability tables to be non-mandatory so users can >> set them directly during frame decoding in cases where no defaults >> have been set previously (eg. ffmpeg vp9 backend) >> * Some comments and documentation clarifications >> * Minor checkpatch fixes >> >> v4: >> >> * Drop color_space field from the VP9 interface. >> V4L2 API should be used for it. >> * Clarified Segment-ID comments. >> * Moved motion vector probabilities to a separate >> struct. >> >> v3: >> >> * Fix documentation issues found by Hans. >> * Fix smatch detected issues as pointed out by Hans. >> * Added patch to fix wrong bytesused set on .buf_prepare. >> >> v2: >> >> * Documentation style issues pointed out by Nicolas internally. >> * s/VP9_PROFILE_MAX/V4L2_VP9_PROFILE_MAX/ >> * Fix wrong kfree(ctx). >> * constify a couple structs on rkvdec-vp9.c >> >> >> Boris Brezillon (2): >> media: uapi: Add VP9 stateless decoder controls >> media: rkvdec: Add the VP9 backend >> >> Ezequiel Garcia (1): >> media: rkvdec: Fix .buf_prepare >> >> .../userspace-api/media/v4l/biblio.rst | 10 + >> .../media/v4l/ext-ctrls-codec.rst | 550 ++++++ >> drivers/media/v4l2-core/v4l2-ctrls.c | 239 +++ >> drivers/media/v4l2-core/v4l2-ioctl.c | 1 + >> drivers/staging/media/rkvdec/Makefile | 2 +- >> drivers/staging/media/rkvdec/rkvdec-vp9.c | 1577 +++++++++++++++++ >> drivers/staging/media/rkvdec/rkvdec.c | 72 +- >> drivers/staging/media/rkvdec/rkvdec.h | 6 + >> include/media/v4l2-ctrls.h | 5 + >> include/media/vp9-ctrls.h | 486 +++++ >> 10 files changed, 2942 insertions(+), 6 deletions(-) >> create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c >> create mode 100644 include/media/vp9-ctrls.h >> >> -- >> 2.29.0 >>
On Wed, 2020-11-11 at 00:28 +0200, Adrian Ratiu wrote: > Hi Ezequiel, > > On Tue, 10 Nov 2020, Ezequiel Garcia > <ezequiel@vanguardiasur.com.ar> wrote: > > On Mon, 2 Nov 2020 at 16:04, Adrian Ratiu > > <adrian.ratiu@collabora.com> wrote: > > > Dear all, > > > > > > This is v5 of the series adding VP9 profile 0 decoding to > > > rkvdec. > > > > > > All feedback from v4 should be addressed, there's just one > > > thing I did not address: ref_frame_sign_biases in the uAPI. The > > > userspace tool I'm > > > > I believe that Hantro G2 VP9 needs ref_frame_sign_biases. > > > > I think that it's also needed for the MTK decoder. Might be > > worth checking that as well, if the code is publicly available > > somewhere. > > I consulted the imx8m app ref manual for the Hantro G2 core and > indeed there's not one, but three fields at SWREG11 and 13 (last, > gold, alt) to signify sign biases for ref frames. Thanks for the > hint! > > > Coming to think about it, I think we are really close to having > > this uAPI directly upstream. > > > > Let's take a step back on why we have these uAPIs in the staging > > area. Couple years ago, there were some doubts in the media > > community about these uAPIs, and we wanted to wait a bit for > > more users before moving to public land. > > > > The uAPIs were meant to be in staging until enough users > > appeared and we were confident enough to move to stable. > > > > For VP9, given the feedback received through the year was > > already addressed, I think all that's left is to check the > > interface and make sure it can support Rockchip (RK3399, RK3326, > > etc), Hantro G2 and Mediatek, > > > > We will be very close to having a public API, and we could even > > merge it directly there. > > Thank you very much for this background. I understand that the > uAPI is independent from the driver implementations, so having a > good stable uAPI is beneficial when (for example) adding support > for VP9 on G2 in hantro or for upstream adoption of these > drivers. > > Given this rkvdec driver implementation is also adding the VP9 > uAPI and it's very close to stability (maybe only missing ref > frame sign bias, but who knows?) would you like to block its > submission until the uAPI is finalized or would it make sense to > treat the uAPI de-staging process separately because the uAPI is > independent from the driver? > I don't mean to block it, quite the opposite, to make sure we take this opportunity to go through Rockchip, Hantro and Mediatek, double-check the uAPI is covering all the VP9 syntax, and then target for public API. Cheers, Ezequiel > Thanks, > Adrian > > > Thanks, > > Ezequiel > > > > > using [1] apparently doesn't need it or the default hwreg value for it > > > is capable of decoding the bitstreams I used on the driver, so I don't > > > really have a use-case to change and test that. :) > > > > > > Considering the uAPI is a work in progress and expected to be modified, > > > ref_frame_sign_biases can be added later with others which might be > > > required to enable more functionality (for eg profiles >= 1). > > > > > > Series tested on rk3399 and applies on next-20201030. > > > > > > [1] https://github.com/Kwiboo/FFmpeg/tree/v4l2-request-hwaccel-4.2.2-rkvdec > > > > > > Changelog > > > --------- > > > > > > v5: > > > > > > * Drop unnecessary OUTPUT buffer payload set in .buf_prepare. > > > * Drop obsolete .per_request ctrl flag > > > * Added new vp9 ctrls to v4l2_ctrl_ptr > > > * Fix pahole detected padding issues > > > * Send userspace an error if it tries to reconfigure decode resolution > > > as v4l2 or rkvdec-vp9 backend do not support dynamic res changes yet > > > * Allow frame ctx probability tables to be non-mandatory so users can > > > set them directly during frame decoding in cases where no defaults > > > have been set previously (eg. ffmpeg vp9 backend) > > > * Some comments and documentation clarifications > > > * Minor checkpatch fixes > > > > > > v4: > > > > > > * Drop color_space field from the VP9 interface. > > > V4L2 API should be used for it. > > > * Clarified Segment-ID comments. > > > * Moved motion vector probabilities to a separate > > > struct. > > > > > > v3: > > > > > > * Fix documentation issues found by Hans. > > > * Fix smatch detected issues as pointed out by Hans. > > > * Added patch to fix wrong bytesused set on .buf_prepare. > > > > > > v2: > > > > > > * Documentation style issues pointed out by Nicolas internally. > > > * s/VP9_PROFILE_MAX/V4L2_VP9_PROFILE_MAX/ > > > * Fix wrong kfree(ctx). > > > * constify a couple structs on rkvdec-vp9.c > > > > > > > > > Boris Brezillon (2): > > > media: uapi: Add VP9 stateless decoder controls > > > media: rkvdec: Add the VP9 backend > > > > > > Ezequiel Garcia (1): > > > media: rkvdec: Fix .buf_prepare > > > > > > .../userspace-api/media/v4l/biblio.rst | 10 + > > > .../media/v4l/ext-ctrls-codec.rst | 550 ++++++ > > > drivers/media/v4l2-core/v4l2-ctrls.c | 239 +++ > > > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + > > > drivers/staging/media/rkvdec/Makefile | 2 +- > > > drivers/staging/media/rkvdec/rkvdec-vp9.c | 1577 +++++++++++++++++ > > > drivers/staging/media/rkvdec/rkvdec.c | 72 +- > > > drivers/staging/media/rkvdec/rkvdec.h | 6 + > > > include/media/v4l2-ctrls.h | 5 + > > > include/media/vp9-ctrls.h | 486 +++++ > > > 10 files changed, 2942 insertions(+), 6 deletions(-) > > > create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c > > > create mode 100644 include/media/vp9-ctrls.h > > > > > > -- > > > 2.29.0 > > >
On Tue, 10 Nov 2020, Ezequiel Garcia <ezequiel@collabora.com> wrote: > On Wed, 2020-11-11 at 00:28 +0200, Adrian Ratiu wrote: >> Hi Ezequiel, >> >> On Tue, 10 Nov 2020, Ezequiel Garcia >> <ezequiel@vanguardiasur.com.ar> wrote: >> > On Mon, 2 Nov 2020 at 16:04, Adrian Ratiu >> > <adrian.ratiu@collabora.com> wrote: >> > > Dear all, This is v5 of the series adding VP9 profile 0 >> > > decoding to rkvdec. All feedback from v4 should be >> > > addressed, there's just one thing I did not address: >> > > ref_frame_sign_biases in the uAPI. The userspace tool I'm >> > I believe that Hantro G2 VP9 needs ref_frame_sign_biases. >> > I think that it's also needed for the MTK decoder. Might be >> > worth checking that as well, if the code is publicly >> > available somewhere. >> I consulted the imx8m app ref manual for the Hantro G2 core >> and indeed there's not one, but three fields at SWREG11 and 13 >> (last, gold, alt) to signify sign biases for ref >> frames. Thanks for the hint! >> >> > Coming to think about it, I think we are really close to >> > having this uAPI directly upstream. Let's take a step >> > back on why we have these uAPIs in the staging area. Couple >> > years ago, there were some doubts in the media community >> > about these uAPIs, and we wanted to wait a bit for more >> > users before moving to public land. The uAPIs were meant >> > to be in staging until enough users appeared and we were >> > confident enough to move to stable. For VP9, given the >> > feedback received through the year was already addressed, I >> > think all that's left is to check the interface and make >> > sure it can support Rockchip (RK3399, RK3326, etc), Hantro >> > G2 and Mediatek, We will be very close to having a public >> > API, and we could even merge it directly there. >> Thank you very much for this background. I understand that the >> uAPI is independent from the driver implementations, so having >> a good stable uAPI is beneficial when (for example) adding >> support for VP9 on G2 in hantro or for upstream adoption of >> these drivers. Given this rkvdec driver implementation is >> also adding the VP9 uAPI and it's very close to stability >> (maybe only missing ref frame sign bias, but who knows?) would >> you like to block its submission until the uAPI is finalized >> or would it make sense to treat the uAPI de-staging process >> separately because the uAPI is independent from the driver? > > I don't mean to block it, quite the opposite, to make sure we > take this opportunity to go through Rockchip, Hantro and > Mediatek, double-check the uAPI is covering all the VP9 syntax, > and then target for public API. That makes sense. I'm just cautious to not directly botch the public API, but that's what reviews are for, right? :) Thanks again for helping with background & direction. > > Cheers, > Ezequiel > >> Thanks, >> Adrian >> >> > Thanks, >> > Ezequiel >> > >> > > using [1] apparently doesn't need it or the default hwreg value for it >> > > is capable of decoding the bitstreams I used on the driver, so I don't >> > > really have a use-case to change and test that. :) >> > > >> > > Considering the uAPI is a work in progress and expected to be modified, >> > > ref_frame_sign_biases can be added later with others which might be >> > > required to enable more functionality (for eg profiles >= 1). >> > > >> > > Series tested on rk3399 and applies on next-20201030. >> > > >> > > [1] https://github.com/Kwiboo/FFmpeg/tree/v4l2-request-hwaccel-4.2.2-rkvdec >> > > >> > > Changelog >> > > --------- >> > > >> > > v5: >> > > >> > > * Drop unnecessary OUTPUT buffer payload set in .buf_prepare. >> > > * Drop obsolete .per_request ctrl flag >> > > * Added new vp9 ctrls to v4l2_ctrl_ptr >> > > * Fix pahole detected padding issues >> > > * Send userspace an error if it tries to reconfigure decode resolution >> > > as v4l2 or rkvdec-vp9 backend do not support dynamic res changes yet >> > > * Allow frame ctx probability tables to be non-mandatory so users can >> > > set them directly during frame decoding in cases where no defaults >> > > have been set previously (eg. ffmpeg vp9 backend) >> > > * Some comments and documentation clarifications >> > > * Minor checkpatch fixes >> > > >> > > v4: >> > > >> > > * Drop color_space field from the VP9 interface. >> > > V4L2 API should be used for it. >> > > * Clarified Segment-ID comments. >> > > * Moved motion vector probabilities to a separate >> > > struct. >> > > >> > > v3: >> > > >> > > * Fix documentation issues found by Hans. >> > > * Fix smatch detected issues as pointed out by Hans. >> > > * Added patch to fix wrong bytesused set on .buf_prepare. >> > > >> > > v2: >> > > >> > > * Documentation style issues pointed out by Nicolas internally. >> > > * s/VP9_PROFILE_MAX/V4L2_VP9_PROFILE_MAX/ >> > > * Fix wrong kfree(ctx). >> > > * constify a couple structs on rkvdec-vp9.c >> > > >> > > >> > > Boris Brezillon (2): >> > > media: uapi: Add VP9 stateless decoder controls >> > > media: rkvdec: Add the VP9 backend >> > > >> > > Ezequiel Garcia (1): >> > > media: rkvdec: Fix .buf_prepare >> > > >> > > .../userspace-api/media/v4l/biblio.rst | 10 + >> > > .../media/v4l/ext-ctrls-codec.rst | 550 ++++++ >> > > drivers/media/v4l2-core/v4l2-ctrls.c | 239 +++ >> > > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + >> > > drivers/staging/media/rkvdec/Makefile | 2 +- >> > > drivers/staging/media/rkvdec/rkvdec-vp9.c | 1577 +++++++++++++++++ >> > > drivers/staging/media/rkvdec/rkvdec.c | 72 +- >> > > drivers/staging/media/rkvdec/rkvdec.h | 6 + >> > > include/media/v4l2-ctrls.h | 5 + >> > > include/media/vp9-ctrls.h | 486 +++++ >> > > 10 files changed, 2942 insertions(+), 6 deletions(-) >> > > create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c >> > > create mode 100644 include/media/vp9-ctrls.h >> > > >> > > -- >> > > 2.29.0 >> > >