| Message ID | 03e0ce22-1ebe-24f4-0f49-f4c41ae8f598@xs4all.nl |
|---|---|
| State | New |
| Headers | show |
| Series | allegro-dvt/nal-h264.h: fix kernel-doc: hdr -> hrd | expand |
Hi Hans, On Tue, 23 Mar 2021 09:33:38 +0100, Hans Verkuil wrote: > On 23/03/2021 09:23, Michael Tretter wrote: > > On Tue, 23 Mar 2021 08:57:53 +0100, Hans Verkuil wrote: > >> On 23/03/2021 08:52, Michael Tretter wrote: > >>> On Tue, 23 Mar 2021 08:49:13 +0100, Hans Verkuil wrote: [...] > >> I think you should either document it all, or change /** to /*. > > > > IMO documenting the parameters is rather pointless, because they are straight > > copies from the specifications and the documentation would be "see H264 > > specification" for every single one of them. I guess, I'll go for changing /** > > to /*. > > > > Actually, I thought about using the sps/pps structs defined in > > include/media/v4l2-ctrls.h. I was not convinced, because these are userspace > > facing API. Are the sps/pps definitions something, that would help other > > drivers, too, or should we rather avoid global definitions to discourage > > sps/pps parsing/generation in drivers? > > There is nothing wrong with using v4l2_ctrl_h264_sps/pps in your driver > instead of your own structs. > > Just note that these structs are now part of the uAPI, so can't be changed. > > If you need allegro specific data as well, then you might be better off > keeping your own structs. I checked this and, unfortunately, I need more data than what is available in the uAPI structs (at least cropping information is missing) and cannot reuse the uAPI structs. > > I'm not sure what you mean with your last question. If a driver needs to do > sps/pps parsing/generation, then why would it matter if it uses global or local > definitions? It still has to do it, right? And having reusable code might help > others. The question arises from the stateful video encoder interface specification [0]: Performing software stream processing, header generation etc. in the driver in order to support this interface is strongly discouraged. In case such operations are needed, use of the Stateless Video Encoder Interface (in development) is strongly advised. However, at least the drivers for the Techwell TW5864 and the CODA at least have partial support for writing sps/pps. I guess, having something reusable would be actually pretty helpful. That being said, the API and the documentation of the current sps/pps parsing/generation code is at least a bit sketchy - inconsistent documentation, h264 function being used for h264 and hevc, etc. I'm adding a cleanup of the interface to my todo list. Maybe at one point we could move this code from the driver to some common directory, if possible. Michael [0] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-encoder.html#memory-to-memory-stateful-video-encoder-interface
diff --git a/drivers/media/platform/allegro-dvt/nal-h264.h b/drivers/media/platform/allegro-dvt/nal-h264.h index 2ba7cbced7a5..8cc5a28bf237 100644 --- a/drivers/media/platform/allegro-dvt/nal-h264.h +++ b/drivers/media/platform/allegro-dvt/nal-h264.h @@ -12,7 +12,7 @@ #include <linux/types.h> /** - * struct nal_h264_hdr_parameters - HDR parameters + * struct nal_h264_hrd_parameters - HRD parameters * * C struct representation of the sequence parameter set NAL unit as defined by * Rec. ITU-T H.264 (04/2017) E.1.2 HRD parameters syntax.
Give typo in kernel-doc documentation: hdr -> hrd Fixes this warning: drivers/media/platform/allegro-dvt/nal-h264.h:33: warning: expecting prototype for struct nal_h264_hdr_parameters. Prototype was for struct nal_h264_hrd_parameters instead Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> ---