Hi Mauro, Thanks for comments! On 5/2/19 4:29 PM, Mauro Carvalho Chehab wrote: > Em Thu, 2 May 2019 15:16:54 +0200 > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu: > >> On 5/2/19 2:55 PM, Mauro Carvalho Chehab wrote: >>> Em Fri, 12 Apr 2019 18:59:15 +0300 >>> Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx> escreveu: >>> >>>> This changes v4l2_pix_format and v4l2_plane_pix_format sizeimage >>>> field description to allow v4l clients to set bigger image size >>>> in case of variable length compressed data. >>>> >>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx> >>>> --- >>>> Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst | 13 ++++++++++++- >>>> Documentation/media/uapi/v4l/pixfmt-v4l2.rst | 11 ++++++++++- >>>> 2 files changed, 22 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst >>>> index 5688c816e334..005428a8121e 100644 >>>> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst >>>> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst >>>> @@ -31,7 +31,18 @@ describing all planes of that format. >>>> >>>> * - __u32 >>>> - ``sizeimage`` >>>> - - Maximum size in bytes required for image data in this plane. >>>> + - Maximum size in bytes required for image data in this plane, >>>> + set by the driver. When the image consists of variable length >>>> + compressed data this is the number of bytes required by the >>>> + codec to support the worst-case compression scenario. >>>> + >>>> + For uncompressed images the driver will set the value. For >>>> + variable length compressed data clients are allowed to set >>>> + the sizeimage field, but the driver may ignore it and set the >>>> + value itself, or it may modify the provided value based on >>>> + alignment requirements or minimum/maximum size requirements. >>>> + If the client wants to leave this to the driver, then it should >>>> + set sizeimage to 0. >>>> * - __u32 >>>> - ``bytesperline`` >>>> - Distance in bytes between the leftmost pixels in two adjacent >>>> diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst >>>> index 71eebfc6d853..0f7771151db9 100644 >>>> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst >>>> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst >>>> @@ -89,7 +89,16 @@ Single-planar format structure >>>> - Size in bytes of the buffer to hold a complete image, set by the >>>> driver. Usually this is ``bytesperline`` times ``height``. When >>>> the image consists of variable length compressed data this is the >>>> - maximum number of bytes required to hold an image. >>>> + number of bytes required by the codec to support the worst-case >>>> + compression scenario. >>>> + >>>> + For uncompressed images the driver will set the value. For >>>> + variable length compressed data clients are allowed to set >>>> + the sizeimage field, but the driver may ignore it and set the >>>> + value itself, or it may modify the provided value based on >>>> + alignment requirements or minimum/maximum size requirements. >>>> + If the client wants to leave this to the driver, then it should >>>> + set sizeimage to 0. >>> >>> It is very confusing to understand what you meant by the above paragraph, >>> as you inverted the sentence order and forgot a comma. >>> >>> I would, instead, write the phrases using the direct order, and break >>> into two paragraphs, e. g., changing the above to: >>> >>> "The driver will set the value for uncompressed images. >>> >>> Clients are allowed to set the sizeimage field for variable length >>> compressed data, but the driver may ignore it and set the >>> value itself, or it may modify the provided value based on >>> alignment requirements or minimum/maximum size requirements. >>> If the client wants to leave this to the driver, then it should >>> set sizeimage to 0." >>> >>> That makes it a lot easier to read, hopefully preventing mistakes from >>> app and driver developers when reading about sizeimage. >>> >>> Yet, I'm not too comfortable on letting this too generic. I mean, >>> how an app writer would know what formats are "variable length >>> compressed data", specially since libv4l may actually change that. >> >> It's actually quite clearly defined: compressed formats set the >> V4L2_FMT_FLAG_COMPRESSED flag in VIDIOC_ENUMFMT. > > Ok, so let's be explicit here, e. g. something like: > > "Clients are allowed to set the sizeimage field for variable length > compressed data flagged with V4L2_FMT_FLAG_COMPRESSED at > VIDIOC_ENUMFMT, but the driver may ignore it and set the > value itself, or it may modify the provided value based on > alignment requirements or minimum/maximum size requirements. > If the client wants to leave this to the driver, then it should > set sizeimage to 0." > > That makes clear for app developers when they can use this new > feature. OK, I will resend with that description. > > That still leads us to what happens at libv4l with sizeimage > for a compressed format that got uncompressed by the library, in > order to ensure that a change like this won't cause breakages at > existing userspace apps. libv4l can decompress formats like MJPEG, right? I mean it isn't to decompress MPEG/H264 for example. -- -- regards, Stan
