Le lundi 26 avril 2021 à 09:38 +0200, Hans Verkuil a écrit : > Hi Andrzej, > > Thank you for working on this! > > On 21/04/2021 12:00, Andrzej Pietrasiewicz wrote: > > Dear All, > > > > This is an RFC on stateless uapi for vp9 decoding with v4l2. This work is based on https://lkml.org/lkml/2020/11/2/1043, but has been substantially reworked. The important change is that the v4l2 control used to pass boolean decoder probabilities has been made unidirectional, and is now called V4L2_CID_STATELESS_VP9_COMPRESSED_HDR_PROBS. > > > > In the previous proposal, to queue a frame the userspace must fully dequeue the previous one, which effectively results in a forced lockstep behavior and defeats vb2's capability to enqueue multiple buffers. Such a design was a consequence of backward probability updates being performed by the kernel driver (which has direct access to appropriate counter values) but forward probability updates being coupled with compressed header parsing performed by the userspace. > > > > In vp9 the boolean decoder used to decode the bitstream needs certain parameters to work. Those are probabilities, which change with each frame. After each frame is decoded it is known how many times a given symbol occured in the frame, so the probabilities can be adapted. This process is known as backward probabilities update. A next frame header can also contain information which modifies probabilities resulting from backward update. The said modification is called forward probabilities update. The data for backward update is generated by the decoder hardware, while the data for forward update is prepared by reading the compressed frame header. The natural place to parse something is userspace, while the natural place to access hardware-provided counters is the kernel. Such responsibilties assignment was used in the original work. > > > > To overcome the lockstep, we moved forward probability updates to the kernel, while leaving parsing them in userspace. This way the v4l2 control which is used to pass the probs becomes unidirectional (user->kernel) and the userspace can keep parsing and enqueueing succeeding frames. > > > > If a particular driver parses the compressed header and does backward probability updates on its own then V4L2_CID_STATELESS_VP9_COMPRESSED_HDR_PROBS does not need to be used. > > > > This series adds vp9 uapi in proper locations, which means it is a proper, "official" uapi, as opposed to staging uapi which was proposed in the above mentioned lkml thread. > > Why? I rather liked the way that the other codec APIs started life in a private header > (like include/media/vp8-ctrls.h) and were given time to mature before moving them to > the uAPI. Is there a reason why you think that VP9 doesn't need that? I'll be honest, I accepted early code into GStreamer for H264, and it ended up in a nightmare for the users. We now have a released GStreamer that supports kernel API up to 5.9, a blackwhole at 5.10 and finally master catched up and can support 5.11+. It is so complicated for packagers to understand what is going on, that they endup wasting a lot of their time for a single feature in their OS. Same breakage is happening for VP8 in 5.13, even though VP8 has been working great all this time. I will for sure for now on ignore any contribution that depends on staged uAPI. As for FFMPEG, even though now H264 API is table, the maintainers just simply ignore the patches as they have been bitten by the reviewing stuff based on unstable APIs and downstream work. I believe the staged uAPI has been used wrongly in the past. Stuff has been staged quicky right before associated project budget for it was exhausted, so it was in the end a way to look good, and someone else had to pick it up and finish it. Going straight for final API put more pressure on making good research from the start, doing more in-depth reviews and avoiding delaying for multiple years the support. I believe the staging API are confusing even for the Linux projects. Going straight to stable here is a commitment to finish this work and doing it correctly. This specially make sense for VP9, which is a very Open CODEC and were all HW implementation are Google/Hantro derivatives. Also, unlike when this work all started, we do have multiple HW we can look at to validate the API, with more then enough in-depth information to make the right decisions. > > > > > The series adds vp9 support to rkvdec driver. > > > > Rebased onto media_tree. > > > > I kindly ask for your comments. > > > > TODO: > > > > - potentially fine-tune the uAPI (add/remove fields, move between structs) > > - write another driver (intended g2 @ iMX8) The commitment is subtly describe here, the commitment is to implement a second driver, Hantro G2 which has a different design), and that even if we have no use for it in the short term. > > - verify the added documentation > > > > Regards, > > > > Andrzej > > > > Andrzej Pietrasiewicz (1): > > media: uapi: Add VP9 stateless decoder controls > > > > Boris Brezillon (1): > > media: rkvdec: Add the VP9 backend > > > > Ezequiel Garcia (1): > > media: rkvdec: Fix .buf_prepare > > Isn't this just a bug fix? Should it be part of this series at all? > > If it is just a bug fix, then please post it separately and let me know if it is > a fix that should go to 5.13 (i.e. the current mainline) or if 5.14 is fine. > > Regards, > > Hans > > > > > .../userspace-api/media/v4l/biblio.rst | 10 + > > .../media/v4l/ext-ctrls-codec-stateless.rst | 523 +++ > > .../media/v4l/pixfmt-compressed.rst | 15 + > > .../media/v4l/vidioc-g-ext-ctrls.rst | 8 + > > .../media/v4l/vidioc-queryctrl.rst | 12 + > > .../media/videodev2.h.rst.exceptions | 2 + > > drivers/media/v4l2-core/v4l2-ctrls.c | 244 ++ > > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + > > drivers/staging/media/rkvdec/Makefile | 2 +- > > drivers/staging/media/rkvdec/rkvdec-vp9.c | 2846 +++++++++++++++++ > > drivers/staging/media/rkvdec/rkvdec.c | 62 +- > > drivers/staging/media/rkvdec/rkvdec.h | 6 + > > include/media/v4l2-ctrls.h | 4 + > > include/uapi/linux/v4l2-controls.h | 455 +++ > > include/uapi/linux/videodev2.h | 6 + > > 15 files changed, 4190 insertions(+), 6 deletions(-) > > create mode 100644 drivers/staging/media/rkvdec/rkvdec-vp9.c > > >
