Add support to more pixel formats in vicodec

The new supported formats are
V4L2_PIX_FMT_GREY, V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.

The returned encoded format is chaned to support various numbers of planes instead of assuming 3 planes.

The first patch adds new fields to structs.
The second patch adds support for V4L2_PIX_FMT_GREY.
The third patch adds support for V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.

Changes from v3:

patch 1,3: - no change

patch 2:
- replace the 2-bit flag FWHT_FL_COMPONENTS_NUM_BIT[01] with GENMASK
- add TODO comment - handle the case where the encoded stream is different format than the decoded
- allocate maximal space for the V4L2_PIX_FMT_FWHT format

with the test 'flags & FWHT_FL_COMPONENTS_NUM_BIT[01]'

Dafna Hirschfeld (3):
media: vicodec: prepare support for various number of planes
media: vicodec: Add support of greyscale format
media: vicodec: Add support for 4 planes formats

 drivers/media/platform/vicodec/codec-fwht.c   |  73 +++++++----
 drivers/media/platform/vicodec/codec-fwht.h   |  15 ++-
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 123 +++++++++++++-----
 .../media/platform/vicodec/codec-v4l2-fwht.h  |   3 +-
 drivers/media/platform/vicodec/vicodec-core.c |  35 ++++-
 5 files changed, 182 insertions(+), 67 deletions(-)

--
2.17.1

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/cover.1541451484.git.dafna3%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Hello Dafna,

Thanks for the patches.

Just out of curiosity. Why these patches havent't been submitted to the media mailing list?

Hi,

Thanks,
Ezequiel

Quoted 10 lines · Dafna Hirschfeld

The new supported formats are
V4L2_PIX_FMT_GREY, V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.

The returned encoded format is chaned to support various numbers of planes instead of assuming 3 planes.

The first patch adds new fields to structs. The second patch adds support for V4L2_PIX_FMT_GREY. The third patch adds support for V4L2_PIX_FMT_ARGB32,

V4L2_PIX_FMT_ABGR32.

Quoted 8 lines

Changes from v3:

patch 1,3: - no change

patch 2:
- replace the 2-bit flag FWHT_FL_COMPONENTS_NUM_BIT[01] with GENMASK
- add TODO comment - handle the case where the encoded stream is

different format

Quoted 17 lines

than the decoded
- allocate maximal space for the V4L2_PIX_FMT_FWHT format

with the test 'flags & FWHT_FL_COMPONENTS_NUM_BIT[01]'

Dafna Hirschfeld (3):
media: vicodec: prepare support for various number of planes
media: vicodec: Add support of greyscale format
media: vicodec: Add support for 4 planes formats

 drivers/media/platform/vicodec/codec-fwht.c   |  73 +++++++----
 drivers/media/platform/vicodec/codec-fwht.h   |  15 ++-
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 123 +++++++++++++-----
 .../media/platform/vicodec/codec-v4l2-fwht.h  |   3 +-
 drivers/media/platform/vicodec/vicodec-core.c |  35 ++++-
 5 files changed, 182 insertions(+), 67 deletions(-)

Quoted 5 lines

--
2.17.1

--
You received this message because you are subscribed to the Google

Groups "outreachy-kernel" group.

Quoted 1 line

To unsubscribe from this group and stop receiving emails from it, send

an email to [email protected].

Quoted 2 lines

To post to this group, send email to [email protected]. To view this discussion on the web visit

https://groups.google.com/d/msgid/outreachy-kernel/cover.1541451484.git.dafna3%40gmail.com
.

Quoted 1 line

For more options, visit https://groups.google.com/d/optout.

--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Quoted 2 lines · Ezequiel Garcia

Hello Dafna,

Quoted 6 lines

Thanks for the patches.

Just out of curiosity. Why these patches havent't been submitted to the media mailing list?

Hi,

I wasn't sure if I should send it to the media mailing list, since this part of outreachy application.

Also, how are you testing these changes?

Quoted 1 line

Based on Helen's decoder:
https://gitlab.collabora.com/koike/v4l2-codec

I extended it to include encoders and decoders for the new supported formats.

testing formats with alpha plane:
https://github.com/kamomil/outreachy/blob/master/argb-and-abgr-full-example.sh

testing greyscale:
https://github.com/kamomil/outreachy/blob/master/greyscale-full-example.sh

Quoted 10 lines · Dafna Hirschfeld

Quoted 7 lines · Ezequiel Garcia

Hello Dafna,

Thanks for the patches.

Just out of curiosity. Why these patches havent't been submitted to the media mailing list?

Quoted 3 lines

Quoted 1 line

Hi,

I wasn't sure if I should send it to the media mailing list, since this part of outreachy application.

In general, for any patch you send to any subsystem please Cc all the relevant mailing lists and maintainers. For Outreachy application you already did that (by Cc'ing Greg), you just need to keep doing the same as you continue your work on other parts of the kernel.

Quoted 6 lines

Also, how are you testing these changes?

Quoted 1 line

Based on Helen's decoder:
https://gitlab.collabora.com/koike/v4l2-codec

Quoted 8 lines

I extended it to include encoders and decoders for the new supported formats.

testing formats with alpha plane:
https://github.com/kamomil/outreachy/blob/master/argb-and-abgr-full-example.sh

testing greyscale:
https://github.com/kamomil/outreachy/blob/master/greyscale-full-example.sh

It's awesome seeing these testsuites, it gives reviewers confidence that your patch is well tested and they can focus on other parts of the review process rather than check for the basic correctness of the patch.

Please include links such as these and indicate how you tested your code in your future patches.

The new supported formats are
V4L2_PIX_FMT_GREY, V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.

The returned encoded format is chaned to support various numbers of planes instead of assuming 3 planes.

The first patch adds new fields to structs.
The second patch adds support for V4L2_PIX_FMT_GREY.
The third patch adds support for V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.

Changes from v3:

patch 1,3: - no change

patch 2:
- replace the 2-bit flag FWHT_FL_COMPONENTS_NUM_BIT[01] with GENMASK
- add TODO comment - handle the case where the encoded stream is different format than the decoded
- allocate maximal space for the V4L2_PIX_FMT_FWHT format

with the test 'flags & FWHT_FL_COMPONENTS_NUM_BIT[01]'

Dafna Hirschfeld (3):
media: vicodec: prepare support for various number of planes
media: vicodec: Add support of greyscale format
media: vicodec: Add support for 4 planes formats

 drivers/media/platform/vicodec/codec-fwht.c   |  73 +++++++----
 drivers/media/platform/vicodec/codec-fwht.h   |  15 ++-
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 123 +++++++++++++-----
 .../media/platform/vicodec/codec-v4l2-fwht.h  |   3 +-
 drivers/media/platform/vicodec/vicodec-core.c |  35 ++++-
 5 files changed, 182 insertions(+), 67 deletions(-)

Hi Dafna,

This patch series looks very nice!

thanks:)

Can you repost this and add a CC to linux-media?

I will

encodes fewer/more planes than the requested raw frame format) is something that needs to be implemented later as part of making vicodec compliant to the stateful codec specification.

I tried to look in the docs what should be done in that case and didn't

Hans

Quoted 9 lines · Dafna Hirschfeld

The new supported formats are
V4L2_PIX_FMT_GREY, V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.

The returned encoded format is chaned to support various numbers of planes instead of assuming 3 planes.

The first patch adds new fields to structs. The second patch adds support for V4L2_PIX_FMT_GREY. The third patch adds support for V4L2_PIX_FMT_ARGB32,

V4L2_PIX_FMT_ABGR32.

Quoted 8 lines

Changes from v3:

patch 1,3: - no change

patch 2:
- replace the 2-bit flag FWHT_FL_COMPONENTS_NUM_BIT[01] with GENMASK
- add TODO comment - handle the case where the encoded stream is

different format

Quoted 17 lines

than the decoded
- allocate maximal space for the V4L2_PIX_FMT_FWHT format

with the test 'flags & FWHT_FL_COMPONENTS_NUM_BIT[01]'

Dafna Hirschfeld (3):
media: vicodec: prepare support for various number of planes
media: vicodec: Add support of greyscale format
media: vicodec: Add support for 4 planes formats

 drivers/media/platform/vicodec/codec-fwht.c   |  73 +++++++----
 drivers/media/platform/vicodec/codec-fwht.h   |  15 ++-
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 123 +++++++++++++-----
 .../media/platform/vicodec/codec-v4l2-fwht.h  |   3 +-
 drivers/media/platform/vicodec/vicodec-core.c |  35 ++++-
 5 files changed, 182 insertions(+), 67 deletions(-)

On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:

Hi Dafna,

This patch series looks very nice!

thanks:)

Can you repost this and add a CC to linux-media?

I will

I'm happy to take this.

The TODO that is in the decoder code (what to do if the fwht bitstream encodes fewer/more planes than the requested raw frame format) is something that needs to be implemented later as part of making vicodec compliant to the stateful codec specification.

I tried to look in the docs what should be done in that case and didn't find.
The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers but this is only optional.

Quoted 16 lines · Dafna Hirschfeld

On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:

Hi Dafna,

This patch series looks very nice!

thanks:)

Can you repost this and add a CC to linux-media?

I will

I'm happy to take this.

Quoted 8 lines

The TODO that is in the decoder code (what to do if the fwht bitstream encodes fewer/more planes than the requested raw frame format) is something that needs to be implemented later as part of making vicodec compliant to the stateful codec specification.

I tried to look in the docs what should be done in that case and didn't find.
The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers but this is only optional.

You are referring to section 4.6.6, point 4 here:

https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html

Right?

It's optional only in the sense that if you already happen to know what the stream contains, then you don't need to do it.

I'm CC-ing Tomasz Figa, since I think just describing a step as 'Optional' is too vague. Something like: 'If <condition>, then you need to perform this step' (or alternatively: 'If !<condition>, then you can skip this step'.

Hi Hans, Dafna,

Quoted 5 lines · Hans Verkuil

Quoted 3 lines · Dafna Hirschfeld

On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected]

Quoted 7 lines · <mailto:[email protected]>> wrote:

Quoted 7 lines

Hi Dafna,

This patch series looks very nice!

thanks:)

Quoted 7 lines

Quoted 7 lines

Can you repost this and add a CC to linux-media?

I will

I'm happy to take this.

The TODO that is in the decoder code (what to do if the fwht

bitstream

Quoted 2 lines

Quoted 2 lines

encodes fewer/more planes than the requested raw frame format) is something that needs to be implemented later as part of making

vicodec

Quoted 3 lines

Quoted 3 lines

compliant to the stateful codec specification.

I tried to look in the docs what should be done in that case and

didn't find.

Quoted 1 line

Quoted 1 line

The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the

supported formats based on the OUTPUT buffers

Quoted 8 lines

Quoted 1 line

but this is only optional.

You are referring to section 4.6.6, point 4 here:

https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html

Right?

Quoted 3 lines

It's optional only in the sense that if you already happen to know what the stream contains, then you don't need to do it.

Quoted 1 line

I'm CC-ing Tomasz Figa, since I think just describing a step as

'Optional'

Quoted 2 lines

is too vague. Something like: 'If <condition>, then you need to perform this step' (or alternatively: 'If !<condition>, then you can skip this

step'.

Just to clarify, the documentation is written from userspace point of view. If something is optional for a userspace application, the application can use it and the driver is expected to support it.

I'm okay replacing "Optional" with something more specific, like "If the client does not need to change the default format", but I wonder if it wouldn't effectively become "if the client does not need to change the default format, or it wants to do it by probing a fixed list of formats it supports by using S_FMT or TRY_FMT, or what not, ...".

Perhaps just explaining the meaning of "Optional" explicitly in the "Conventions and notation used in this document" would be good enough?

Best regards,
Tomasz

On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <[email protected] <mailto:[email protected]>> wrote:

    Hi Hans, Dafna,

    On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:
    >
    > On 11/15/18 12:14, Dafna Hirschfeld wrote:
    > >
    > >
    > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:
    > >
    > >     Hi Dafna,
    > >
    > >     This patch series looks very nice!
    > >
    > > thanks:)
    > >
    > >     Can you repost this and add a CC to linux-media?
    > >
    > > I will
    > >
    > >     I'm happy to take this.
    > >
    > >     The TODO that is in the decoder code (what to do if the fwht bitstream
    > >     encodes fewer/more planes than the requested raw frame format) is
    > >     something that needs to be implemented later as part of making vicodec
    > >     compliant to the stateful codec specification.
    > >
    > > I tried to look in the docs what should be done in that case and didn't find.
    > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
    > > but this is only optional.
    >
    > You are referring to section 4.6.6, point 4 here:
    >
    > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
    >
    > Right?
    >

Right,

Quoted 3 lines

It's optional only in the sense that if you already happen to know what the stream contains, then you don't need to do it.

It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?

Dafna

Quoted 3 lines

I'm CC-ing Tomasz Figa, since I think just describing a step as 'Optional' is too vague. Something like: 'If <condition>, then you need to perform this step' (or alternatively: 'If !<condition>, then you can skip this step'.

Just to clarify, the documentation is written from userspace point of view. If something is optional for a userspace application, the application can use it and the driver is expected to support it.

I'm okay replacing "Optional" with something more specific, like "If the client does not need to change the default format", but I wonder if it wouldn't effectively become "if the client does not need to change the default format, or it wants to do it by probing a fixed list of formats it supports by using S_FMT or TRY_FMT, or what not, ...".

Perhaps just explaining the meaning of "Optional" explicitly in the "Conventions and notation used in this document" would be good enough?

Best regards,
Tomasz

Quoted 44 lines · Dafna Hirschfeld

On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <[email protected] <mailto:[email protected]>> wrote:

    Hi Hans, Dafna,

    On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:
    >
    > On 11/15/18 12:14, Dafna Hirschfeld wrote:
    > >
    > >
    > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:
    > >
    > >     Hi Dafna,
    > >
    > >     This patch series looks very nice!
    > >
    > > thanks:)
    > >
    > >     Can you repost this and add a CC to linux-media?
    > >
    > > I will
    > >
    > >     I'm happy to take this.
    > >
    > >     The TODO that is in the decoder code (what to do if the fwht bitstream
    > >     encodes fewer/more planes than the requested raw frame format) is
    > >     something that needs to be implemented later as part of making vicodec
    > >     compliant to the stateful codec specification.
    > >
    > > I tried to look in the docs what should be done in that case and didn't find.
    > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
    > > but this is only optional.
    >
    > You are referring to section 4.6.6, point 4 here:
    >
    > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
    >
    > Right?
    >

Right,

Quoted 6 lines

Quoted 3 lines

It's optional only in the sense that if you already happen to know what the stream contains, then you don't need to do it.

It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?

No, we can't trust users. At all :-)

So the driver would have to act the same as if there was a resolution change.

Tomasz, this is actually something to improve: it is not just resolution changes, but also incompatible capture formats. In this case, the stream contains RGB, but the capture format is YUV, and vicodec cannot convert from RGB to YUV.

I think sending a SOURCE_CHANGE event makes sense. One thing though: this would take place at the very start of the decoding process, so step 2 in section 4.6.9 (Dynamic resolution change) says "The decoder will then process and decode all remaining buffers from before the resolution change point.". But no buffers have been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?

It is probably the right thing to do.

Quoted 67 lines · Hans Verkuil

Quoted 44 lines · Dafna Hirschfeld

On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <[email protected] <mailto:[email protected]>> wrote:

    Hi Hans, Dafna,

    On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:
    >
    > On 11/15/18 12:14, Dafna Hirschfeld wrote:
    > >
    > >
    > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:
    > >
    > >     Hi Dafna,
    > >
    > >     This patch series looks very nice!
    > >
    > > thanks:)
    > >
    > >     Can you repost this and add a CC to linux-media?
    > >
    > > I will
    > >
    > >     I'm happy to take this.
    > >
    > >     The TODO that is in the decoder code (what to do if the fwht bitstream
    > >     encodes fewer/more planes than the requested raw frame format) is
    > >     something that needs to be implemented later as part of making vicodec
    > >     compliant to the stateful codec specification.
    > >
    > > I tried to look in the docs what should be done in that case and didn't find.
    > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
    > > but this is only optional.
    >
    > You are referring to section 4.6.6, point 4 here:
    >
    > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
    >
    > Right?
    >

Right,

Quoted 6 lines

Quoted 3 lines

It's optional only in the sense that if you already happen to know what the stream contains, then you don't need to do it.

It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?

No, we can't trust users. At all :-)

So the driver would have to act the same as if there was a resolution change.

Tomasz, this is actually something to improve: it is not just resolution changes, but also incompatible capture formats. In this case, the stream contains RGB, but the capture format is YUV, and vicodec cannot convert from RGB to YUV.

I think sending a SOURCE_CHANGE event makes sense. One thing though: this would take place at the very start of the decoding process, so step 2 in section 4.6.9 (Dynamic resolution change) says "The decoder will then process and decode all remaining buffers from before the resolution change point.". But no buffers have been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?

Quoted 1 line

It is probably the right thing to do.

There is already a source change event specified to be signaled in the Initialization sequence. It's defined to always happen after the driver establishes the format. Given that, is there any change still needed?

Quoted 70 lines · Tomasz Figa

Quoted 68 lines · Hans Verkuil

Quoted 44 lines · Dafna Hirschfeld

On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <[email protected] <mailto:[email protected]>> wrote:

    Hi Hans, Dafna,

    On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:
    >
    > On 11/15/18 12:14, Dafna Hirschfeld wrote:
    > >
    > >
    > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:
    > >
    > >     Hi Dafna,
    > >
    > >     This patch series looks very nice!
    > >
    > > thanks:)
    > >
    > >     Can you repost this and add a CC to linux-media?
    > >
    > > I will
    > >
    > >     I'm happy to take this.
    > >
    > >     The TODO that is in the decoder code (what to do if the fwht bitstream
    > >     encodes fewer/more planes than the requested raw frame format) is
    > >     something that needs to be implemented later as part of making vicodec
    > >     compliant to the stateful codec specification.
    > >
    > > I tried to look in the docs what should be done in that case and didn't find.
    > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
    > > but this is only optional.
    >
    > You are referring to section 4.6.6, point 4 here:
    >
    > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
    >
    > Right?
    >

Right,

Quoted 6 lines

Quoted 3 lines

It's optional only in the sense that if you already happen to know what the stream contains, then you don't need to do it.

It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?

No, we can't trust users. At all :-)

So the driver would have to act the same as if there was a resolution change.

Tomasz, this is actually something to improve: it is not just resolution changes, but also incompatible capture formats. In this case, the stream contains RGB, but the capture format is YUV, and vicodec cannot convert from RGB to YUV.

I think sending a SOURCE_CHANGE event makes sense. One thing though: this would take place at the very start of the decoding process, so step 2 in section 4.6.9 (Dynamic resolution change) says "The decoder will then process and decode all remaining buffers from before the resolution change point.". But no buffers have been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?

It is probably the right thing to do.

Quoted 4 lines

There is already a source change event specified to be signaled in the Initialization sequence. It's defined to always happen after the driver establishes the format. Given that, is there any change still needed?

Ah, you're right. It has to be documented that this happens as part of the pixel format documentation, right? So this should be added to the FWHT format doc once the proper behavior is implemented.

Quoted 72 lines · Hans Verkuil

Quoted 70 lines · Tomasz Figa

Quoted 68 lines · Hans Verkuil

Quoted 44 lines · Dafna Hirschfeld

On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <[email protected] <mailto:[email protected]>> wrote:

    Hi Hans, Dafna,

    On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:
    >
    > On 11/15/18 12:14, Dafna Hirschfeld wrote:
    > >
    > >
    > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:
    > >
    > >     Hi Dafna,
    > >
    > >     This patch series looks very nice!
    > >
    > > thanks:)
    > >
    > >     Can you repost this and add a CC to linux-media?
    > >
    > > I will
    > >
    > >     I'm happy to take this.
    > >
    > >     The TODO that is in the decoder code (what to do if the fwht bitstream
    > >     encodes fewer/more planes than the requested raw frame format) is
    > >     something that needs to be implemented later as part of making vicodec
    > >     compliant to the stateful codec specification.
    > >
    > > I tried to look in the docs what should be done in that case and didn't find.
    > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
    > > but this is only optional.
    >
    > You are referring to section 4.6.6, point 4 here:
    >
    > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
    >
    > Right?
    >

Right,

Quoted 6 lines

Quoted 3 lines

It's optional only in the sense that if you already happen to know what the stream contains, then you don't need to do it.

It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?

No, we can't trust users. At all :-)

So the driver would have to act the same as if there was a resolution change.

Tomasz, this is actually something to improve: it is not just resolution changes, but also incompatible capture formats. In this case, the stream contains RGB, but the capture format is YUV, and vicodec cannot convert from RGB to YUV.

I think sending a SOURCE_CHANGE event makes sense. One thing though: this would take place at the very start of the decoding process, so step 2 in section 4.6.9 (Dynamic resolution change) says "The decoder will then process and decode all remaining buffers from before the resolution change point.". But no buffers have been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?

It is probably the right thing to do.

Quoted 8 lines

Quoted 4 lines

There is already a source change event specified to be signaled in the Initialization sequence. It's defined to always happen after the driver establishes the format. Given that, is there any change still needed?

Ah, you're right. It has to be documented that this happens as part of the pixel format documentation, right? So this should be added to the FWHT format doc once the proper behavior is implemented.

I wonder if it wouldn't be more consistent if we just made the driver send this event even for streams that don't include format data in the headers. In that case the driver would already have the information about the format from the OUTPUT queue (and internal constraints), so it could just signal the event instantly. WDYT?

Quoted 82 lines · Tomasz Figa

Quoted 72 lines · Hans Verkuil

Quoted 70 lines · Tomasz Figa

Quoted 68 lines · Hans Verkuil

Quoted 44 lines · Dafna Hirschfeld

On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <[email protected] <mailto:[email protected]>> wrote:

    Hi Hans, Dafna,

    On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:
    >
    > On 11/15/18 12:14, Dafna Hirschfeld wrote:
    > >
    > >
    > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:
    > >
    > >     Hi Dafna,
    > >
    > >     This patch series looks very nice!
    > >
    > > thanks:)
    > >
    > >     Can you repost this and add a CC to linux-media?
    > >
    > > I will
    > >
    > >     I'm happy to take this.
    > >
    > >     The TODO that is in the decoder code (what to do if the fwht bitstream
    > >     encodes fewer/more planes than the requested raw frame format) is
    > >     something that needs to be implemented later as part of making vicodec
    > >     compliant to the stateful codec specification.
    > >
    > > I tried to look in the docs what should be done in that case and didn't find.
    > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
    > > but this is only optional.
    >
    > You are referring to section 4.6.6, point 4 here:
    >
    > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
    >
    > Right?
    >

Right,

Quoted 6 lines

Quoted 3 lines

It's optional only in the sense that if you already happen to know what the stream contains, then you don't need to do it.

It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?

No, we can't trust users. At all :-)

So the driver would have to act the same as if there was a resolution change.

Tomasz, this is actually something to improve: it is not just resolution changes, but also incompatible capture formats. In this case, the stream contains RGB, but the capture format is YUV, and vicodec cannot convert from RGB to YUV.

I think sending a SOURCE_CHANGE event makes sense. One thing though: this would take place at the very start of the decoding process, so step 2 in section 4.6.9 (Dynamic resolution change) says "The decoder will then process and decode all remaining buffers from before the resolution change point.". But no buffers have been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?

It is probably the right thing to do.

Quoted 8 lines

Quoted 4 lines

There is already a source change event specified to be signaled in the Initialization sequence. It's defined to always happen after the driver establishes the format. Given that, is there any change still needed?

Ah, you're right. It has to be documented that this happens as part of the pixel format documentation, right? So this should be added to the FWHT format doc once the proper behavior is implemented.

Quoted 5 lines

I wonder if it wouldn't be more consistent if we just made the driver send this event even for streams that don't include format data in the headers. In that case the driver would already have the information about the format from the OUTPUT queue (and internal constraints), so it could just signal the event instantly. WDYT?

Can't we leave this open? Are there codecs where the stream doesn't have meta data? I'm not aware of any.

I'm not sure if we need to mention codecs without embedded meta data at all.

Perhaps just mention that such codecs are unsupported at the moment.

Quoted 93 lines · Hans Verkuil

Quoted 82 lines · Tomasz Figa

Quoted 72 lines · Hans Verkuil

Quoted 70 lines · Tomasz Figa

Quoted 68 lines · Hans Verkuil

Quoted 44 lines · Dafna Hirschfeld

On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <[email protected] <mailto:[email protected]>> wrote:

    Hi Hans, Dafna,

    On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:
    >
    > On 11/15/18 12:14, Dafna Hirschfeld wrote:
    > >
    > >
    > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:
    > >
    > >     Hi Dafna,
    > >
    > >     This patch series looks very nice!
    > >
    > > thanks:)
    > >
    > >     Can you repost this and add a CC to linux-media?
    > >
    > > I will
    > >
    > >     I'm happy to take this.
    > >
    > >     The TODO that is in the decoder code (what to do if the fwht bitstream
    > >     encodes fewer/more planes than the requested raw frame format) is
    > >     something that needs to be implemented later as part of making vicodec
    > >     compliant to the stateful codec specification.
    > >
    > > I tried to look in the docs what should be done in that case and didn't find.
    > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
    > > but this is only optional.
    >
    > You are referring to section 4.6.6, point 4 here:
    >
    > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
    >
    > Right?
    >

Right,

Quoted 6 lines

Quoted 3 lines

It's optional only in the sense that if you already happen to know what the stream contains, then you don't need to do it.

It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?

No, we can't trust users. At all :-)

So the driver would have to act the same as if there was a resolution change.

Tomasz, this is actually something to improve: it is not just resolution changes, but also incompatible capture formats. In this case, the stream contains RGB, but the capture format is YUV, and vicodec cannot convert from RGB to YUV.

I think sending a SOURCE_CHANGE event makes sense. One thing though: this would take place at the very start of the decoding process, so step 2 in section 4.6.9 (Dynamic resolution change) says "The decoder will then process and decode all remaining buffers from before the resolution change point.". But no buffers have been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?

It is probably the right thing to do.

Quoted 8 lines

Quoted 4 lines

There is already a source change event specified to be signaled in the Initialization sequence. It's defined to always happen after the driver establishes the format. Given that, is there any change still needed?

Ah, you're right. It has to be documented that this happens as part of the pixel format documentation, right? So this should be added to the FWHT format doc once the proper behavior is implemented.

Quoted 5 lines

I wonder if it wouldn't be more consistent if we just made the driver send this event even for streams that don't include format data in the headers. In that case the driver would already have the information about the format from the OUTPUT queue (and internal constraints), so it could just signal the event instantly. WDYT?

Can't we leave this open? Are there codecs where the stream doesn't have meta data? I'm not aware of any.

Quoted 3 lines

I'm not sure if we need to mention codecs without embedded meta data at all.

Perhaps just mention that such codecs are unsupported at the moment.

That would indeed simplify things. I don't know too much about codecs other than H.264 and VPx, so can't really tell if none of the codecs we have FourCCs for would qualify as such, though...

Quoted 98 lines · Tomasz Figa

Quoted 96 lines · Hans Verkuil

Quoted 82 lines · Tomasz Figa

Quoted 72 lines · Hans Verkuil

Quoted 70 lines · Tomasz Figa

Quoted 68 lines · Hans Verkuil

Quoted 44 lines · Dafna Hirschfeld

On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <[email protected] <mailto:[email protected]>> wrote:

    Hi Hans, Dafna,

    On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:
    >
    > On 11/15/18 12:14, Dafna Hirschfeld wrote:
    > >
    > >
    > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:
    > >
    > >     Hi Dafna,
    > >
    > >     This patch series looks very nice!
    > >
    > > thanks:)
    > >
    > >     Can you repost this and add a CC to linux-media?
    > >
    > > I will
    > >
    > >     I'm happy to take this.
    > >
    > >     The TODO that is in the decoder code (what to do if the fwht bitstream
    > >     encodes fewer/more planes than the requested raw frame format) is
    > >     something that needs to be implemented later as part of making vicodec
    > >     compliant to the stateful codec specification.
    > >
    > > I tried to look in the docs what should be done in that case and didn't find.
    > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
    > > but this is only optional.
    >
    > You are referring to section 4.6.6, point 4 here:
    >
    > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
    >
    > Right?
    >

Right,

Quoted 6 lines

Quoted 3 lines

It's optional only in the sense that if you already happen to know what the stream contains, then you don't need to do it.

It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?

No, we can't trust users. At all :-)

So the driver would have to act the same as if there was a resolution change.

Tomasz, this is actually something to improve: it is not just resolution changes, but also incompatible capture formats. In this case, the stream contains RGB, but the capture format is YUV, and vicodec cannot convert from RGB to YUV.

I think sending a SOURCE_CHANGE event makes sense. One thing though: this would take place at the very start of the decoding process, so step 2 in section 4.6.9 (Dynamic resolution change) says "The decoder will then process and decode all remaining buffers from before the resolution change point.". But no buffers have been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?

It is probably the right thing to do.

Quoted 8 lines

Quoted 4 lines

There is already a source change event specified to be signaled in the Initialization sequence. It's defined to always happen after the driver establishes the format. Given that, is there any change still needed?

Ah, you're right. It has to be documented that this happens as part of the pixel format documentation, right? So this should be added to the FWHT format doc once the proper behavior is implemented.

Quoted 5 lines

I wonder if it wouldn't be more consistent if we just made the driver send this event even for streams that don't include format data in the headers. In that case the driver would already have the information about the format from the OUTPUT queue (and internal constraints), so it could just signal the event instantly. WDYT?

Can't we leave this open? Are there codecs where the stream doesn't have meta data? I'm not aware of any.

I'm not sure if we need to mention codecs without embedded meta data at all.

Perhaps just mention that such codecs are unsupported at the moment.

Quoted 3 lines

That would indeed simplify things. I don't know too much about codecs other than H.264 and VPx, so can't really tell if none of the codecs we have FourCCs for would qualify as such, though...

As far as I am aware, all of them do have metadata in the bitstream. So I'd just leave it out from the spec. Or perhaps leave a single mention that such codecs are not supported at the moment.

Quoted 37 lines · Tomasz Figa

Hi Hans, Dafna,

Quoted 33 lines · Hans Verkuil

Quoted 16 lines · Dafna Hirschfeld

On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:

Hi Dafna,

This patch series looks very nice!

thanks:)

Can you repost this and add a CC to linux-media?

I will

I'm happy to take this.

Quoted 8 lines

The TODO that is in the decoder code (what to do if the fwht bitstream encodes fewer/more planes than the requested raw frame format) is something that needs to be implemented later as part of making vicodec compliant to the stateful codec specification.

I tried to look in the docs what should be done in that case and didn't find.
The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers but this is only optional.

You are referring to section 4.6.6, point 4 here:

https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html

Right?

Right,

Quoted 1 line

Quoted 3 lines

Quoted 3 lines

It's optional only in the sense that if you already happen to know what the stream contains, then you don't need to do it.

It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?

Dafna

Quoted 21 lines

Quoted 4 lines

I'm CC-ing Tomasz Figa, since I think just describing a step as 'Optional' is too vague. Something like: 'If <condition>, then you need to perform this step' (or alternatively: 'If !<condition>, then you can skip this step'.

Just to clarify, the documentation is written from userspace point of view. If something is optional for a userspace application, the application can use it and the driver is expected to support it.

I'm okay replacing "Optional" with something more specific, like "If the client does not need to change the default format", but I wonder if it wouldn't effectively become "if the client does not need to change the default format, or it wants to do it by probing a fixed list of formats it supports by using S_FMT or TRY_FMT, or what not, ...".

Perhaps just explaining the meaning of "Optional" explicitly in the "Conventions and notation used in this document" would be good enough?

Best regards,
Tomasz