| Message ID | 20200106154929.4331-5-stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Venus new features | expand |
On 1/6/20 4:49 PM, Stanimir Varbanov wrote: > This event indicate that the source color 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 color bit-depth is changed from 8 to 10bits > or vice versa. > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > Documentation/media/uapi/v4l/vidioc-dqevent.rst | 8 +++++++- > Documentation/media/videodev2.h.rst.exceptions | 1 + > include/uapi/linux/videodev2.h | 1 + > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst > index 42659a3d1705..fad853d440cf 100644 > --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst > +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst > @@ -402,7 +402,13 @@ call. > that many Video Capture 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_COLOR_DEPTH`` > + - 0x0002 > + - This event gets triggered when color 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>`). I think this is too specific for decoders. Something similar to the CH_RESOLUTION description would be more appropriate: - This event gets triggered when a color bit-depth change (but not a resolution change!) is detected at an input. This can come from an input connector or from a video decoder. Applications will have to query the new pixel format and re-negotiate the queue. For stateful decoders follow the guidelines in :ref:`decoder`. Video capture devices will in most cases have to stop and restart streaming (:ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` followed by :ref:`VIDIOC_STREAMON <VIDIOC_STREAMON>`). And update dev-decoder.rst where needed with this new event flag. As to your question on irc: once I've acked this patch it can be merged via a venus PR. Regards, Hans > > Return Value > ============ > diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions > index cb6ccf91776e..209709114378 100644 > --- a/Documentation/media/videodev2.h.rst.exceptions > +++ b/Documentation/media/videodev2.h.rst.exceptions > @@ -490,6 +490,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_COLOR_DEPTH 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 5f9357dcb060..1d349c9d57a7 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -2332,6 +2332,7 @@ struct v4l2_event_frame_sync { > }; > > #define V4L2_EVENT_SRC_CH_RESOLUTION (1 << 0) > +#define V4L2_EVENT_SRC_CH_COLOR_DEPTH (1 << 1) > > struct v4l2_event_src_change { > __u32 changes; >
On 1/8/20 6:09 PM, Hans Verkuil wrote: > On 1/6/20 4:49 PM, Stanimir Varbanov wrote: >> This event indicate that the source color 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 color bit-depth is changed from 8 to 10bits >> or vice versa. >> >> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> Documentation/media/uapi/v4l/vidioc-dqevent.rst | 8 +++++++- >> Documentation/media/videodev2.h.rst.exceptions | 1 + >> include/uapi/linux/videodev2.h | 1 + >> 3 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >> index 42659a3d1705..fad853d440cf 100644 >> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst >> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >> @@ -402,7 +402,13 @@ call. >> that many Video Capture 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_COLOR_DEPTH`` >> + - 0x0002 >> + - This event gets triggered when color 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>`). > > I think this is too specific for decoders. Something similar to the > CH_RESOLUTION description would be more appropriate: > > - This event gets triggered when a color bit-depth change (but not a > resolution change!) is detected at an input. This can come from an > input connector or from a video decoder. Applications will have to query > the new pixel format and re-negotiate the queue. > > For stateful decoders follow the guidelines in :ref:`decoder`. > Video capture devices will in most cases have to stop and restart > streaming (:ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` followed by > :ref:`VIDIOC_STREAMON <VIDIOC_STREAMON>`). > > And update dev-decoder.rst where needed with this new event flag. > > As to your question on irc: once I've acked this patch it can be merged > via a venus PR. Sounds good, thank you.
Hi Hans, On 1/8/20 6:09 PM, Hans Verkuil wrote: > On 1/6/20 4:49 PM, Stanimir Varbanov wrote: >> This event indicate that the source color 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 color bit-depth is changed from 8 to 10bits >> or vice versa. >> >> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> Documentation/media/uapi/v4l/vidioc-dqevent.rst | 8 +++++++- >> Documentation/media/videodev2.h.rst.exceptions | 1 + >> include/uapi/linux/videodev2.h | 1 + >> 3 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >> index 42659a3d1705..fad853d440cf 100644 >> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst >> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >> @@ -402,7 +402,13 @@ call. >> that many Video Capture 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_COLOR_DEPTH`` >> + - 0x0002 >> + - This event gets triggered when color 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>`). > > I think this is too specific for decoders. Something similar to the > CH_RESOLUTION description would be more appropriate: > > - This event gets triggered when a color bit-depth change (but not a > resolution change!) is detected at an input. This can come from an What you mean by "but not a resolution change" here? Resolution change and bit-depth change cannot occur on the same time, or something else. I would say that for Venus (and probably others) on initialization time both could be changed on the same time, because we cannot predict the resolution and bit-depth before parsing bitstream headers. > input connector or from a video decoder. Applications will have to query > the new pixel format and re-negotiate the queue. > > For stateful decoders follow the guidelines in :ref:`decoder`. > Video capture devices will in most cases have to stop and restart > streaming (:ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` followed by > :ref:`VIDIOC_STREAMON <VIDIOC_STREAMON>`). > > And update dev-decoder.rst where needed with this new event flag. > > As to your question on irc: once I've acked this patch it can be merged > via a venus PR. > > Regards, > > Hans > >> >> Return Value >> ============ >> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions >> index cb6ccf91776e..209709114378 100644 >> --- a/Documentation/media/videodev2.h.rst.exceptions >> +++ b/Documentation/media/videodev2.h.rst.exceptions >> @@ -490,6 +490,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_COLOR_DEPTH 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 5f9357dcb060..1d349c9d57a7 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -2332,6 +2332,7 @@ struct v4l2_event_frame_sync { >> }; >> >> #define V4L2_EVENT_SRC_CH_RESOLUTION (1 << 0) >> +#define V4L2_EVENT_SRC_CH_COLOR_DEPTH (1 << 1) >> >> struct v4l2_event_src_change { >> __u32 changes; >> >
On 1/9/20 8:41 AM, Stanimir Varbanov wrote: > Hi Hans, > > On 1/8/20 6:09 PM, Hans Verkuil wrote: >> On 1/6/20 4:49 PM, Stanimir Varbanov wrote: >>> This event indicate that the source color 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 color bit-depth is changed from 8 to 10bits >>> or vice versa. >>> >>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >>> --- >>> Documentation/media/uapi/v4l/vidioc-dqevent.rst | 8 +++++++- >>> Documentation/media/videodev2.h.rst.exceptions | 1 + >>> include/uapi/linux/videodev2.h | 1 + >>> 3 files changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>> index 42659a3d1705..fad853d440cf 100644 >>> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>> @@ -402,7 +402,13 @@ call. >>> that many Video Capture 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_COLOR_DEPTH`` >>> + - 0x0002 >>> + - This event gets triggered when color 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>`). >> >> I think this is too specific for decoders. Something similar to the >> CH_RESOLUTION description would be more appropriate: >> >> - This event gets triggered when a color bit-depth change (but not a >> resolution change!) is detected at an input. This can come from an > > What you mean by "but not a resolution change" here? Resolution change > and bit-depth change cannot occur on the same time, or something else. What I was trying to say is that a resolution change implies a possible bit-depth change as well, whereas V4L2_EVENT_SRC_CH_COLOR_DEPTH is only set if there is a bit-depth change but no resolution change. V4L2_EVENT_SRC_CH_RESOLUTION requires that userspace does a full resync to the new format, CH_COLOR_DEPTH implies that only the bit depth changed. Which actually makes me wonder: is there a difference between the two change flags w.r.t. userspace behavior? If there is, then that should be carefully documented, if there isn't, then is this new flag really needed? Regards, Hans > > I would say that for Venus (and probably others) on initialization time > both could be changed on the same time, because we cannot predict the > resolution and bit-depth before parsing bitstream headers. > >> input connector or from a video decoder. Applications will have to query >> the new pixel format and re-negotiate the queue. >> >> For stateful decoders follow the guidelines in :ref:`decoder`. >> Video capture devices will in most cases have to stop and restart >> streaming (:ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` followed by >> :ref:`VIDIOC_STREAMON <VIDIOC_STREAMON>`). >> >> And update dev-decoder.rst where needed with this new event flag. >> >> As to your question on irc: once I've acked this patch it can be merged >> via a venus PR. >> >> Regards, >> >> Hans >> >>> >>> Return Value >>> ============ >>> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions >>> index cb6ccf91776e..209709114378 100644 >>> --- a/Documentation/media/videodev2.h.rst.exceptions >>> +++ b/Documentation/media/videodev2.h.rst.exceptions >>> @@ -490,6 +490,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_COLOR_DEPTH 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 5f9357dcb060..1d349c9d57a7 100644 >>> --- a/include/uapi/linux/videodev2.h >>> +++ b/include/uapi/linux/videodev2.h >>> @@ -2332,6 +2332,7 @@ struct v4l2_event_frame_sync { >>> }; >>> >>> #define V4L2_EVENT_SRC_CH_RESOLUTION (1 << 0) >>> +#define V4L2_EVENT_SRC_CH_COLOR_DEPTH (1 << 1) >>> >>> struct v4l2_event_src_change { >>> __u32 changes; >>> >> >
Hi Hans, On 1/9/20 10:57 AM, Hans Verkuil wrote: > On 1/9/20 8:41 AM, Stanimir Varbanov wrote: >> Hi Hans, >> >> On 1/8/20 6:09 PM, Hans Verkuil wrote: >>> On 1/6/20 4:49 PM, Stanimir Varbanov wrote: >>>> This event indicate that the source color 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 color bit-depth is changed from 8 to 10bits >>>> or vice versa. >>>> >>>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >>>> --- >>>> Documentation/media/uapi/v4l/vidioc-dqevent.rst | 8 +++++++- >>>> Documentation/media/videodev2.h.rst.exceptions | 1 + >>>> include/uapi/linux/videodev2.h | 1 + >>>> 3 files changed, 9 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>>> index 42659a3d1705..fad853d440cf 100644 >>>> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>>> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>>> @@ -402,7 +402,13 @@ call. >>>> that many Video Capture 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_COLOR_DEPTH`` >>>> + - 0x0002 >>>> + - This event gets triggered when color 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>`). >>> >>> I think this is too specific for decoders. Something similar to the >>> CH_RESOLUTION description would be more appropriate: >>> >>> - This event gets triggered when a color bit-depth change (but not a >>> resolution change!) is detected at an input. This can come from an >> >> What you mean by "but not a resolution change" here? Resolution change >> and bit-depth change cannot occur on the same time, or something else. > > What I was trying to say is that a resolution change implies a possible bit-depth > change as well, whereas V4L2_EVENT_SRC_CH_COLOR_DEPTH is only set if there is > a bit-depth change but no resolution change. > > V4L2_EVENT_SRC_CH_RESOLUTION requires that userspace does a full resync to the > new format, CH_COLOR_DEPTH implies that only the bit depth changed. CH_COLOR_DEPTH implies format re-negotiation as well. In Venus case 8->10bit change will change the format of OPB buffers (now it is not possible because of lack of v4l modifiers) and DPB buffers becomes compressed raw buffers (to optimize bandwidth). > > Which actually makes me wonder: is there a difference between the two change flags > w.r.t. userspace behavior? If there is, then that should be carefully documented, > if there isn't, then is this new flag really needed? Looking into semantics of v4l events, CH_COLOR_DEPTH makes sense because it describes what actually changed (similar to CH_RESOLUTION). I would say that v4l2_event::type V4L2_EVENT_SOURCE_CHANGE implies format re-negotiation and v4l2_event::src_change just informs userland what exactly is changed. I'll postpone this patch until we have clear vision what will be the usage in user-space.
On 1/10/20 11:54 AM, Stanimir Varbanov wrote: > Hi Hans, > > On 1/9/20 10:57 AM, Hans Verkuil wrote: >> On 1/9/20 8:41 AM, Stanimir Varbanov wrote: >>> Hi Hans, >>> >>> On 1/8/20 6:09 PM, Hans Verkuil wrote: >>>> On 1/6/20 4:49 PM, Stanimir Varbanov wrote: >>>>> This event indicate that the source color 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 color bit-depth is changed from 8 to 10bits >>>>> or vice versa. >>>>> >>>>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >>>>> --- >>>>> Documentation/media/uapi/v4l/vidioc-dqevent.rst | 8 +++++++- >>>>> Documentation/media/videodev2.h.rst.exceptions | 1 + >>>>> include/uapi/linux/videodev2.h | 1 + >>>>> 3 files changed, 9 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>>>> index 42659a3d1705..fad853d440cf 100644 >>>>> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>>>> @@ -402,7 +402,13 @@ call. >>>>> that many Video Capture 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_COLOR_DEPTH`` >>>>> + - 0x0002 >>>>> + - This event gets triggered when color 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>`). >>>> >>>> I think this is too specific for decoders. Something similar to the >>>> CH_RESOLUTION description would be more appropriate: >>>> >>>> - This event gets triggered when a color bit-depth change (but not a >>>> resolution change!) is detected at an input. This can come from an >>> >>> What you mean by "but not a resolution change" here? Resolution change >>> and bit-depth change cannot occur on the same time, or something else. >> >> What I was trying to say is that a resolution change implies a possible bit-depth >> change as well, whereas V4L2_EVENT_SRC_CH_COLOR_DEPTH is only set if there is >> a bit-depth change but no resolution change. >> >> V4L2_EVENT_SRC_CH_RESOLUTION requires that userspace does a full resync to the >> new format, CH_COLOR_DEPTH implies that only the bit depth changed. > > CH_COLOR_DEPTH implies format re-negotiation as well. In Venus case > 8->10bit change will change the format of OPB buffers (now it is not > possible because of lack of v4l modifiers) and DPB buffers becomes > compressed raw buffers (to optimize bandwidth). > >> >> Which actually makes me wonder: is there a difference between the two change flags >> w.r.t. userspace behavior? If there is, then that should be carefully documented, >> if there isn't, then is this new flag really needed? > > Looking into semantics of v4l events, CH_COLOR_DEPTH makes sense because > it describes what actually changed (similar to CH_RESOLUTION). I would > say that v4l2_event::type V4L2_EVENT_SOURCE_CHANGE implies format > re-negotiation and v4l2_event::src_change just informs userland what > exactly is changed. > > I'll postpone this patch until we have clear vision what will be the > usage in user-space. > My main problem regarding semantics is what should be done if you disconnect and reconnect an HDMI (for example) connector. You get a V4L2_EVENT_SOURCE_CHANGE when the new signal is detected, but should it just set V4L2_EVENT_SRC_CH_RESOLUTION (as it does today), or also V4L2_EVENT_SRC_CH_COLOR_DEPTH? In my view V4L2_EVENT_SRC_CH_COLOR_DEPTH only makes sense if the resolution stays the same, but only the color depth changes. BTW, one thing that will be added here as well in the future is a V4L2_EVENT_SRC_CH_COLORIMETRY for when the colorspace etc. information changes, but nothing else. In that case there is no need to renegotiate the format etc., it's just the interpretation of the video data that changes. An alternative approach is to define a V4L2_EVENT_SRC_CH_ALL bit mask, OR-ing all the V4L2_EVENT_SRC_CH_* defines, and change all drivers that use V4L2_EVENT_SRC_CH_RESOLUTION at the moment to use V4L2_EVENT_SRC_CH_ALL instead, and only drivers that detect that only one of these changes took place will use a specific V4L2_EVENT_SRC_CH_ flag. This will be primarily codec drivers, I believe. There aren't that many of those, so this shouldn't be too difficult to do. Perhaps this is the cleanest approach to this problem... Regards, Hans
Hi Hans, On 1/15/20 5:03 PM, Hans Verkuil wrote: > On 1/10/20 11:54 AM, Stanimir Varbanov wrote: >> Hi Hans, >> >> On 1/9/20 10:57 AM, Hans Verkuil wrote: >>> On 1/9/20 8:41 AM, Stanimir Varbanov wrote: >>>> Hi Hans, >>>> >>>> On 1/8/20 6:09 PM, Hans Verkuil wrote: >>>>> On 1/6/20 4:49 PM, Stanimir Varbanov wrote: >>>>>> This event indicate that the source color 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 color bit-depth is changed from 8 to 10bits >>>>>> or vice versa. >>>>>> >>>>>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >>>>>> --- >>>>>> Documentation/media/uapi/v4l/vidioc-dqevent.rst | 8 +++++++- >>>>>> Documentation/media/videodev2.h.rst.exceptions | 1 + >>>>>> include/uapi/linux/videodev2.h | 1 + >>>>>> 3 files changed, 9 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>>>>> index 42659a3d1705..fad853d440cf 100644 >>>>>> --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>>>>> +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst >>>>>> @@ -402,7 +402,13 @@ call. >>>>>> that many Video Capture 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_COLOR_DEPTH`` >>>>>> + - 0x0002 >>>>>> + - This event gets triggered when color 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>`). >>>>> >>>>> I think this is too specific for decoders. Something similar to the >>>>> CH_RESOLUTION description would be more appropriate: >>>>> >>>>> - This event gets triggered when a color bit-depth change (but not a >>>>> resolution change!) is detected at an input. This can come from an >>>> >>>> What you mean by "but not a resolution change" here? Resolution change >>>> and bit-depth change cannot occur on the same time, or something else. >>> >>> What I was trying to say is that a resolution change implies a possible bit-depth >>> change as well, whereas V4L2_EVENT_SRC_CH_COLOR_DEPTH is only set if there is >>> a bit-depth change but no resolution change. >>> >>> V4L2_EVENT_SRC_CH_RESOLUTION requires that userspace does a full resync to the >>> new format, CH_COLOR_DEPTH implies that only the bit depth changed. >> >> CH_COLOR_DEPTH implies format re-negotiation as well. In Venus case >> 8->10bit change will change the format of OPB buffers (now it is not >> possible because of lack of v4l modifiers) and DPB buffers becomes >> compressed raw buffers (to optimize bandwidth). >> >>> >>> Which actually makes me wonder: is there a difference between the two change flags >>> w.r.t. userspace behavior? If there is, then that should be carefully documented, >>> if there isn't, then is this new flag really needed? >> >> Looking into semantics of v4l events, CH_COLOR_DEPTH makes sense because >> it describes what actually changed (similar to CH_RESOLUTION). I would >> say that v4l2_event::type V4L2_EVENT_SOURCE_CHANGE implies format >> re-negotiation and v4l2_event::src_change just informs userland what >> exactly is changed. >> >> I'll postpone this patch until we have clear vision what will be the >> usage in user-space. >> > > My main problem regarding semantics is what should be done if you disconnect > and reconnect an HDMI (for example) connector. You get a V4L2_EVENT_SOURCE_CHANGE > when the new signal is detected, but should it just set V4L2_EVENT_SRC_CH_RESOLUTION > (as it does today), or also V4L2_EVENT_SRC_CH_COLOR_DEPTH? I think disconnect -> connect should imply that everything is changed i.e. source resolution, color depth and colorimetry. We cannot guess is it the same HDMI source or not. > > In my view V4L2_EVENT_SRC_CH_COLOR_DEPTH only makes sense if the resolution > stays the same, but only the color depth changes. I can imagine a bitstream which changing resolution and color-depth at the same time. And I guess h264|265 doesn't denied this to happen. > > BTW, one thing that will be added here as well in the future is a > V4L2_EVENT_SRC_CH_COLORIMETRY for when the colorspace etc. information changes, > but nothing else. In that case there is no need to renegotiate the format etc., > it's just the interpretation of the video data that changes. In case of colorimetry, maybe the buffer format will not change but colorspace, quantization, ycbcr_encoding and transfer function will change. And this imply that the userspace have to streamoff -> gfmt -> do-some-action -> streamon ? I have to figure out what is for example Android is doing when the colorimentry is changed. If someone knows, please shed some light on that subject. > > An alternative approach is to define a V4L2_EVENT_SRC_CH_ALL bit mask, OR-ing > all the V4L2_EVENT_SRC_CH_* defines, and change all drivers that use > V4L2_EVENT_SRC_CH_RESOLUTION at the moment to use V4L2_EVENT_SRC_CH_ALL instead, > and only drivers that detect that only one of these changes took place will > use a specific V4L2_EVENT_SRC_CH_ flag. This will be primarily codec drivers, > I believe. I can do that as an RFC. > > There aren't that many of those, so this shouldn't be too difficult to do. > > Perhaps this is the cleanest approach to this problem... > > Regards, > > Hans >
diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst index 42659a3d1705..fad853d440cf 100644 --- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst +++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst @@ -402,7 +402,13 @@ call. that many Video Capture 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_COLOR_DEPTH`` + - 0x0002 + - This event gets triggered when color 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 cb6ccf91776e..209709114378 100644 --- a/Documentation/media/videodev2.h.rst.exceptions +++ b/Documentation/media/videodev2.h.rst.exceptions @@ -490,6 +490,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_COLOR_DEPTH 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 5f9357dcb060..1d349c9d57a7 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -2332,6 +2332,7 @@ struct v4l2_event_frame_sync { }; #define V4L2_EVENT_SRC_CH_RESOLUTION (1 << 0) +#define V4L2_EVENT_SRC_CH_COLOR_DEPTH (1 << 1) struct v4l2_event_src_change { __u32 changes;