linux-kernel - Re: [PATCH] v4l: Add source event change for bit-depth

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a61ded35-2dc1-edb9-9f9a-94bbf78c7abf@xs4all.nl>
Date:   Wed, 24 Apr 2019 12:23:06 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Stanimir Varbanov <stanimir.varbanov@...aro.org>
Cc:     linux-media@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH] v4l: Add source event change for bit-depth

On 4/23/19 8:43 AM, Sakari Ailus wrote:
> Hi Stan,
> 
> On Mon, Apr 15, 2019 at 06:27:23PM +0300, Stanimir Varbanov wrote:
>> This event indicate that the source bit-depth is changed during
>> run-time. The client must get the new format and re-allocate buffers
>> for it. This can usually happens with video decoder (encoders) when
>> the bit-stream depth is changed from 8 to 10bits or vice versa.
> 
> Thanks for the patch.
> 
> How about adding an event for pixelformat change, instead of bit depth? The
> application presumably knows the formats it can work with, and can adapt to
> that as well as a change to the bit depth.

I disagree with this. The pixelformat is set by the application, and it is
not set by the incoming video format. Of course, a change in the incoming
video format might force a pixelformat change, but that's not what is signaled
here. This event just indicates that something changed w.r.t. the video source
and the flag specifies that is it the bit depth.

So I think this patch is OK (but see the small comment below) since it
correctly describes what changed.

> 
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@...aro.org>
>> ---
>>  Documentation/media/uapi/v4l/vidioc-dqevent.rst | 7 +++++++
>>  Documentation/media/videodev2.h.rst.exceptions  | 1 +
>>  include/uapi/linux/videodev2.h                  | 1 +
>>  3 files changed, 9 insertions(+)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>> index dea9c0cc00ab..92573d7c3e05 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
>> @@ -397,6 +397,13 @@ call.
>>  	that many devices are not able to recover from a temporary loss of
>>  	signal and so restarting streaming I/O is required in order for the
>>  	hardware to synchronize to the video signal.
>> +    * - ``V4L2_EVENT_SRC_CH_BITDEPTH``
>> +      - 0x0002
>> +      - This event gets triggered when bit-depth change is detected from a
>> +	video decoder. Applications will have to query the new pixel format
>> +	and re-negotiate the queue. In most cases the streaming must be stopped
>> +	and restarted (:ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` followed by
>> +	:ref:`VIDIOC_STREAMON <VIDIOC_STREAMON>`).
>>  
>>  
>>  Return Value
>> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
>> index 64d348e67df9..b4c35b347983 100644
>> --- a/Documentation/media/videodev2.h.rst.exceptions
>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>> @@ -478,6 +478,7 @@ replace define V4L2_EVENT_CTRL_CH_FLAGS ctrl-changes-flags
>>  replace define V4L2_EVENT_CTRL_CH_RANGE ctrl-changes-flags
>>  
>>  replace define V4L2_EVENT_SRC_CH_RESOLUTION src-changes-flags
>> +replace define V4L2_EVENT_SRC_CH_BITDEPTH src-changes-flags
>>  
>>  replace define V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ :c:type:`v4l2_event_motion_det`
>>  
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 496e6453450c..7398f42eac7d 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -2271,6 +2271,7 @@ struct v4l2_event_frame_sync {
>>  };
>>  
>>  #define V4L2_EVENT_SRC_CH_RESOLUTION		(1 << 0)
>> +#define V4L2_EVENT_SRC_CH_BITDEPTH		(1 << 1)

It's two words, so BIT_DEPTH.

For the future I would also like to see a V4L2_EVENT_SRC_CH_COLORIMETRY flag
to indicate when the colorimetry of the video changes.

It's actually something that would be of use for vivid today.

Regards,

	Hans

>>  
>>  struct v4l2_event_src_change {
>>  	__u32 changes;
>