[GIT PULL] Selection API and fixes for v3.2

[GIT PULL] Selection API and fixes for v3.2

[Marek Szyprowski]

Hello Mauro,

I've collected pending selection API patches together with pending
videobuf2 and Samsung driver fixes to a single git branch. Please pull
them to your media tree.

Best regards,
Marek Szyprowski
Samsung Poland R&D Center

The following changes since commit 699cc1962c85351689c27dd46e598e4204fdd105:

  [media] TT-budget S2-3200 cannot tune on HB13E DVBS2 transponder (2011-09-21 17:06:56 -0300)

are available in the git repository at:
  git://git.infradead.org/users/kmpark/linux-2.6-samsung for_mauro

Hatim Ali (1):
      s5p-tv: Add PM_RUNTIME dependency

Marek Szyprowski (1):
      staging: dt3155v4l: fix build break

Michael Olbrich (1):
      v4l: mem2mem: add wait_{prepare,finish} ops to m2m_testdev

Scott Jiang (1):
      vb2: add vb2_get_unmapped_area in vb2 core

Sylwester Nawrocki (1):
      m5mols: Remove superfluous irq field from the platform data struct

Tomasz Stanislawski (6):
      v4l: add support for selection api
      v4l: add documentation for selection API
      v4l: emulate old crop API using extended crop/compose API
      v4l: s5p-tv: mixer: add support for selection API
      s5p-tv: hdmi: use DVI mode
      s5p-tv: fix mbus configuration

 Documentation/DocBook/media/constraints.png.b64    |  134 +
 Documentation/DocBook/media/selection.png.b64      | 2937 ++++++++++++++++++++
 Documentation/DocBook/media/v4l/common.xml         |    4 +-
 Documentation/DocBook/media/v4l/selection-api.xml  |  278 ++
 Documentation/DocBook/media/v4l/v4l2.xml           |    1 +
 .../DocBook/media/v4l/vidioc-g-selection.xml       |  281 ++
 drivers/media/video/m5mols/m5mols_core.c           |    6 +-
 drivers/media/video/mem2mem_testdev.c              |   14 +
 drivers/media/video/s5p-tv/Kconfig                 |    2 +-
 drivers/media/video/s5p-tv/hdmi_drv.c              |   15 +-
 drivers/media/video/s5p-tv/mixer.h                 |   14 +-
 drivers/media/video/s5p-tv/mixer_grp_layer.c       |  157 +-
 drivers/media/video/s5p-tv/mixer_reg.c             |   11 +-
 drivers/media/video/s5p-tv/mixer_video.c           |  339 ++-
 drivers/media/video/s5p-tv/mixer_vp_layer.c        |  108 +-
 drivers/media/video/s5p-tv/regs-hdmi.h             |    4 +
 drivers/media/video/s5p-tv/regs-mixer.h            |    1 +
 drivers/media/video/s5p-tv/sdo_drv.c               |    1 +
 drivers/media/video/v4l2-compat-ioctl32.c          |    2 +
 drivers/media/video/v4l2-ioctl.c                   |  114 +-
 drivers/media/video/videobuf2-core.c               |   31 +
 drivers/staging/dt3155v4l/dt3155v4l.c              |    4 +-
 include/linux/videodev2.h                          |   46 +
 include/media/m5mols.h                             |    4 +-
 include/media/v4l2-ioctl.h                         |    4 +
 include/media/videobuf2-core.h                     |    7 +
 26 files changed, 4292 insertions(+), 227 deletions(-)
 create mode 100644 Documentation/DocBook/media/constraints.png.b64
 create mode 100644 Documentation/DocBook/media/selection.png.b64
 create mode 100644 Documentation/DocBook/media/v4l/selection-api.xml
 create mode 100644 Documentation/DocBook/media/v4l/vidioc-g-selection.xml

Re: [GIT PULL] Selection API and fixes for v3.2

[Mauro Carvalho Chehab]

Em 22-09-2011 12:13, Marek Szyprowski escreveu:
> Hello Mauro,
> 
> I've collected pending selection API patches together with pending
> videobuf2 and Samsung driver fixes to a single git branch. Please pull
> them to your media tree.
> 

> Marek Szyprowski (1):
>       staging: dt3155v4l: fix build break

I've applied this one previously, from the patch you sent me.

 
> Tomasz Stanislawski (6):
>       v4l: add support for selection api
>       v4l: add documentation for selection API

I need more time to review those two patches. I'll probably do it at the next week.
I generally start analyzing API changes based on the DocBook, so, let me point a few
things I've noticed on a quick read, at the vidioc-g-selection.html DocBook-generated page:

1) "The coordinates are expressed in driver-dependant units"

Why? coordinates should be expressed in pixels, as otherwise there's no way to
use this API on a hardware-independent way.

2)
    0 - driver is free to adjust size, it is recommended to choose the crop/compose rectangle as close as possible to the original one

    SEL_SIZE_GE - driver is not allowed to shrink the rectangle. The original rectangle must lay inside the adjusted one

    SEL_SIZE_LE - drive is not allowed to grow the rectangle. The adjusted rectangle must lay inside the original one

    SEL_SIZE_GE | SEL_SIZE_LE - choose size exactly the same as in desired rectangle.

The macro names above don't match the definition, as they aren't prefixed by V4L2_.

3) There was no hyperlink for the struct v4l2_selection, as on other API definitions.

4) the language doesn't seem too consistent with the way other ioctl's are defined. For example,
you're using struct::field for a field at the struct. Other parts of the API just say "field foo of struct bar".

5) There's not a single mention at the git commit or at the DocBook about why the old crop API
is being deprecated. You need to convince me about such need (ok, I followed a few discussions in
the past, but, my brain patch buffer is shorter than the 7000 patchwork patches I reviewed just on
this week). Besides that: do we really need to obsolete the crop API for TV cards? If so, why? If not,
you need to explain why a developer should opt between one ioctl set of the other.

6) You should add a note about it at hist-v4l2.html page, stating what happened, and why a new crop
ioctl set is needed.

7) You didn't update the Experimental API Elements or the Obsolete API Elements at the hist-v4l2.html

Thanks,
Mauro

Re: [GIT PULL] Selection API and fixes for v3.2

[Tomasz Stanislawski]

On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:

Hi Mauro
Thank you for your comments. Please refer to the answers below.
> Em 22-09-2011 12:13, Marek Szyprowski escreveu:
>> Hello Mauro,
>>
>> I've collected pending selection API patches together with pending
>> videobuf2 and Samsung driver fixes to a single git branch. Please pull
>> them to your media tree.
>>
>
>> Marek Szyprowski (1):
>>        staging: dt3155v4l: fix build break
>
> I've applied this one previously, from the patch you sent me.
>
>
>> Tomasz Stanislawski (6):
>>        v4l: add support for selection api
>>        v4l: add documentation for selection API
>
> I need more time to review those two patches. I'll probably do it at the next week.
> I generally start analyzing API changes based on the DocBook, so, let me point a few
> things I've noticed on a quick read, at the vidioc-g-selection.html DocBook-generated page:
>
> 1) "The coordinates are expressed in driver-dependant units"
>
> Why? coordinates should be expressed in pixels, as otherwise there's no way to
> use this API on a hardware-independent way.
The documentation for existing cropping API contains following sentence:

"To support a wide range of hardware this specification does not define 
an origin or units."

I decided to follow the same convention for the selection API.
I thought that the only exception would be images in memory buffers, 
whose coordinated would be pixels.

However, as Laurent mentioned, some devices are capable of sub-pixel 
cropping. Moreover, there are image exotic formats that have no 
well-defined resolution (fractal or vector graphics). Now I am still not 
sure if the requirement for resolution in pixels should be used any 
more. The problem is that this requirement is very intuitive and useful 
in "let's say" 95% of cases.

>
> 2)
>      0 - driver is free to adjust size, it is recommended to choose the crop/compose rectangle as close as possible to the original one
>
>      SEL_SIZE_GE - driver is not allowed to shrink the rectangle. The original rectangle must lay inside the adjusted one
>
>      SEL_SIZE_LE - drive is not allowed to grow the rectangle. The adjusted rectangle must lay inside the original one
>
>      SEL_SIZE_GE | SEL_SIZE_LE - choose size exactly the same as in desired rectangle.
>
> The macro names above don't match the definition, as they aren't prefixed by V4L2_.
ok.. I will fix it.
>
> 3) There was no hyperlink for the struct v4l2_selection, as on other API definitions.
ok.. I will fix it.
>
> 4) the language doesn't seem too consistent with the way other ioctl's are defined. For example,
> you're using struct::field for a field at the struct. Other parts of the API just say "field foo of struct bar".
ok.. I will fix it.

> 5) There's not a single mention at the git commit or at the DocBook about why the old crop API
> is being deprecated. You need to convince me about such need (ok, I followed a few discussions in
> the past, but, my brain patch buffer is shorter than the 7000 patchwork patches I reviewed just on
> this week). Besides that: do we really need to obsolete the crop API for TV cards? If so, why? If not,
> you need to explain why a developer should opt between one ioctl set of the other.

There are a few reasons to drop support for the cropping ioctls.

First, the selection API covers existing crop API. Therefore there is no 
need to implement {S/G}_CROP and G_CROPCAP. The {S/G}_SELECTION is 
enough to provide the same functionality. We should avoiding duplication 
inside the api, therefore S_CROP should be dropped.

Second, there is a patch that adds simulation of old crop API using 
selection API. Therefore there is no need to change code of the existing 
applications.

Third, the selection is much more powerful API, and it could be extended 
in future. There no more reserved fields inside structures
used by current cropping api. Moreover cropping is very inconsistent. 
For output devices cropping into display actually means composing into 
display. Moreover it not possible to select only the part of the image 
from the buffer to be inserted or filled by the hardware. The selection 
API introduced the idea of the constraints flags, that are used for 
precise control of the coordinates' rounding policy.

I could split commit 'v4l: add documentation for selection API' into two 
commits. One that deprecates S_CROP, and another one that introduces 
selection.

>
> 6) You should add a note about it at hist-v4l2.html page, stating what happened, and why a new crop
> ioctl set is needed.
ok.. I missed it. Sorry.
>
> 7) You didn't update the Experimental API Elements or the Obsolete API Elements at the hist-v4l2.html
ok.. I missed it. Sorry.
>
> Thanks,
> Mauro

Thank again for your comments.
I hope that my answers will convince you to the selection API.

Best regards,
Tomasz Stanislawski

RE: [GIT PULL] Selection API and fixes for v3.2

[Marek Szyprowski]

Hello Mauro,

Ok, I agree that s5p-tv patches need some more work. What about
the other patches, especially these two:
"vb2: add vb2_get_unmapped_area in vb2 core"
"v4l: mem2mem: add wait_{prepare,finish} ops to m2m_testdev"? 

I cannot find them in your staging/for_v3.2 branch yet - should
I include them in the next pull request?

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [GIT PULL] Selection API and fixes for v3.2

[Mauro Carvalho Chehab]

Em 22-09-2011 12:13, Marek Szyprowski escreveu:
> Hello Mauro,
> 
> I've collected pending selection API patches together with pending
> videobuf2 and Samsung driver fixes to a single git branch. Please pull
> them to your media tree.
> 
> Best regards,
> Marek Szyprowski
> Samsung Poland R&D Center
> 
> The following changes since commit 699cc1962c85351689c27dd46e598e4204fdd105:
> 
>   [media] TT-budget S2-3200 cannot tune on HB13E DVBS2 transponder (2011-09-21 17:06:56 -0300)
> 
> are available in the git repository at:
>   git://git.infradead.org/users/kmpark/linux-2.6-samsung for_mauro

Continuing the patches review from this series:

0689133 [media] s5p-tv: fix mbus configuration
17b2747 [media] s5p-tv: hdmi: use DVI mode
0f6c565 [media] s5p-tv: Add PM_RUNTIME dependency

Applied, thanks!

> Scott Jiang (1):
>       vb2: add vb2_get_unmapped_area in vb2 core

> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index ea55c08..977410b 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -309,6 +309,13 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type);
>  int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type);
>  
>  int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma);
> +#ifndef CONFIG_MMU
> +unsigned long vb2_get_unmapped_area(struct vb2_queue *q,
> +				    unsigned long addr,
> +				    unsigned long len,
> +				    unsigned long pgoff,
> +				    unsigned long flags);
> +#endif
>  unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait);
>  size_t vb2_read(struct vb2_queue *q, char __user *data, size_t count,
>  		loff_t *ppos, int nonblock);

This sounds me like a hack, as it is passing the problem of working with a non-mmu
capable hardware to the driver, inserting architecture-dependent bits on them.

The proper way to do it is to provide a vb2 core support to handle the non-mmu case 
inside it.

Thanks,
Mauro

Re: [GIT PULL] Selection API and fixes for v3.2

[Mauro Carvalho Chehab]

Em 26-09-2011 07:03, Marek Szyprowski escreveu:
> Hello Mauro,
> 
> Ok, I agree that s5p-tv patches need some more work. What about
> the other patches, especially these two:
> "vb2: add vb2_get_unmapped_area in vb2 core"

Just reviewed this one.

> "v4l: mem2mem: add wait_{prepare,finish} ops to m2m_testdev"? 

Applied, thanks.

> I cannot find them in your staging/for_v3.2 branch yet - should
> I include them in the next pull request?

I reserved some fun to work on this Monday morning ;)

Anyway, I've reviewed the remaining patches already.

Thanks,
Mauro

RE: [GIT PULL] Selection API and fixes for v3.2

[Marek Szyprowski]

Hello,

On Monday, September 26, 2011 1:14 PM Mauro Carvalho Chehab wrote:

> > Scott Jiang (1):
> >       vb2: add vb2_get_unmapped_area in vb2 core
> 
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index ea55c08..977410b 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -309,6 +309,13 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type);
> >  int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type);
> >
> >  int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma);
> > +#ifndef CONFIG_MMU
> > +unsigned long vb2_get_unmapped_area(struct vb2_queue *q,
> > +				    unsigned long addr,
> > +				    unsigned long len,
> > +				    unsigned long pgoff,
> > +				    unsigned long flags);
> > +#endif
> >  unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait);
> >  size_t vb2_read(struct vb2_queue *q, char __user *data, size_t count,
> >  		loff_t *ppos, int nonblock);
> 
> This sounds me like a hack, as it is passing the problem of working with a non-mmu
> capable hardware to the driver, inserting architecture-dependent bits on them.
> 
> The proper way to do it is to provide a vb2 core support to handle the non-mmu case
> inside it.

This is exactly what this patch does - it provides generic vb2 implementation for 
fops->get_unmapped_area callback which any vb2 ready driver can use. This operation
is used only on NON-MMU systems. Please check drivers/media/video/v4l2-dev.c file and
the implementation of get_unmapped_area there. Similar code is used by uvc driver.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [GIT PULL] Selection API and fixes for v3.2

[Mauro Carvalho Chehab]

Em 26-09-2011 05:42, Tomasz Stanislawski escreveu:
> On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:
> 
> Hi Mauro
> Thank you for your comments. Please refer to the answers below.
>> Em 22-09-2011 12:13, Marek Szyprowski escreveu:
>>> Hello Mauro,
>>>
>>> I've collected pending selection API patches together with pending
>>> videobuf2 and Samsung driver fixes to a single git branch. Please pull
>>> them to your media tree.
>>>
>>
>>> Marek Szyprowski (1):
>>>        staging: dt3155v4l: fix build break
>>
>> I've applied this one previously, from the patch you sent me.
>>
>>
>>> Tomasz Stanislawski (6):
>>>        v4l: add support for selection api
>>>        v4l: add documentation for selection API
>>
>> I need more time to review those two patches. I'll probably do it at the next week.
>> I generally start analyzing API changes based on the DocBook, so, let me point a few
>> things I've noticed on a quick read, at the vidioc-g-selection.html DocBook-generated page:
>>
>> 1) "The coordinates are expressed in driver-dependant units"
>>
>> Why? coordinates should be expressed in pixels, as otherwise there's no way to
>> use this API on a hardware-independent way.
> The documentation for existing cropping API contains following sentence:
> 
> "To support a wide range of hardware this specification does not define an origin or units."
> 
> I decided to follow the same convention for the selection API.
> I thought that the only exception would be images in memory buffers, whose coordinated would be pixels.
> 
> However, as Laurent mentioned, some devices are capable of sub-pixel cropping. Moreover, there are image exotic formats that have no well-defined resolution (fractal or vector graphics). 
> Now I am still not sure if the requirement for resolution in pixels should be used any more. The problem is that this requirement is very intuitive and useful in "let's say" 95% of cases.

How an userspace application is supposed to know the type of scale?
I can't see any way of querying the scale type.

>>
>> 2)
>>      0 - driver is free to adjust size, it is recommended to choose the crop/compose rectangle as close as possible to the original one
>>
>>      SEL_SIZE_GE - driver is not allowed to shrink the rectangle. The original rectangle must lay inside the adjusted one
>>
>>      SEL_SIZE_LE - drive is not allowed to grow the rectangle. The adjusted rectangle must lay inside the original one
>>
>>      SEL_SIZE_GE | SEL_SIZE_LE - choose size exactly the same as in desired rectangle.
>>
>> The macro names above don't match the definition, as they aren't prefixed by V4L2_.
> ok.. I will fix it.
>>
>> 3) There was no hyperlink for the struct v4l2_selection, as on other API definitions.
> ok.. I will fix it.
>>
>> 4) the language doesn't seem too consistent with the way other ioctl's are defined. For example,
>> you're using struct::field for a field at the struct. Other parts of the API just say "field foo of struct bar".
> ok.. I will fix it.
> 
>> 5) There's not a single mention at the git commit or at the DocBook about why the old crop API
>> is being deprecated. You need to convince me about such need (ok, I followed a few discussions in
>> the past, but, my brain patch buffer is shorter than the 7000 patchwork patches I reviewed just on
>> this week). Besides that: do we really need to obsolete the crop API for TV cards? If so, why? If not,
>> you need to explain why a developer should opt between one ioctl set of the other.
> 
> There are a few reasons to drop support for the cropping ioctls.
> 
> First, the selection API covers existing crop API. Therefore there is no need to implement {S/G}_CROP and G_CROPCAP. 
> The {S/G}_SELECTION is enough to provide the same functionality. We should avoiding duplication inside the api, 
> therefore S_CROP should be dropped.
> 
> Second, there is a patch that adds simulation of old crop API using selection API. Therefore there is no need to 
> change code of the existing applications.

Both are fine, but you should notice that they aren't arguments why an
userspace application or a driver shouldn't implement/use the crop API.

> Third, the selection is much more powerful API, and it could be extended in future. There no more reserved fields inside structures
> used by current cropping api.

Ok.

> Moreover cropping is very inconsistent.

Why? Please describe its inconsistencies at the DocBook.

> For output devices cropping into display actually means composing into display.

Ok.

> Moreover it not possible to select only 
> the part of the image from the buffer to be inserted or filled by the hardware. 

You're talking again on output devices, right?


> The selection API introduced the idea 
> of the constraints flags, that are used for precise control of the coordinates' rounding policy.

Ok, but I fail to see where a rounding policy would be
needed on an input device.

In other words, I think we should split the issues with 
the crop api into two groups:

1) for input devices
2) for output devices.

Are your idea to deprecate the usage of the crop API for both
input and output devices? 

Please correct me if I'm wrong, but the problems you've mentioned are all
about (2), right?

The crop API were designed originally for input devices, and is currently
used on several TV and USB webcam devices for input. 
Grepping for vidioc_s_crap:
	drivers/media/video/bt8xx/bttv-driver.c
	drivers/media/video/et61x251/et61x251_core.c
	drivers/media/video/saa7134/saa7134-video.c
	drivers/media/video/sn9c102/sn9c102_core.c
	drivers/media/video/vino.c
	drivers/media/video/zoran/zoran_driver.c
	drivers/media/video/cx18/cx18-ioctl.c

Porting them to the selection API means that userspace applications
and kernel drivers will need changes, and this will take a long time
to happen. Ok, this can be done, if there will be large gains on it,
but I fail to see what will be such gains.

So, please explain us why the above drivers would need to be ported to
the selection API, or why new input drivers/applications would need
the new API instead of the old one.

With respect to (2), the only TV device that (ab)used the crop API to 
control its output node is
	drivers/media/video/ivtv/ivtv-ioctl.c

To complete the drivers list, currently, the only SoC device currently
implementing vidioc_s_crop for input/output is the davinci driver:
	drivers/media/video/davinci/vpif_capture.c
	drivers/media/video/davinci/vpif_display.c

IMO, what it seems to be happening with the crop API is similar to what
happened in the past with the control API: the existing API works fine
on simple cases, but fails on more complex scenarios. In the case of
the control API, several controls need to be grouped when selecting an
mpeg compression parameters. So, the VIDIOC_[G|S]_EXT_CTRLS were added
without deprecating the old ioctl's. This way, applications that were
only supporting controls like bright, volume, etc won't need to be changed.

Internally, it made sense to implement a core way of converting a legacy
ioctl call into the new callbacks for the _EXT_ ioctl's, and to convert
the existing drivers.

> I could split commit 'v4l: add documentation for selection API' into two commits. One that deprecates S_CROP, and another one that introduces selection.

Actually, It is too soon to deprecate S_CROP while the selection API is tagged
as experimental, but if this is the idea, it is better to add a hint at the V4L2
DocBook.

>> 6) You should add a note about it at hist-v4l2.html page, stating what happened, and why a new crop
>> ioctl set is needed.
> ok.. I missed it. Sorry.
>>
>> 7) You didn't update the Experimental API Elements or the Obsolete API Elements at the hist-v4l2.html
> ok.. I missed it. Sorry.
>>
>> Thanks,
>> Mauro
> 
> Thank again for your comments.
> I hope that my answers will convince you to the selection API.
> 
> Best regards,
> Tomasz Stanislawski
> 

Regards,
Mauro

Re: [GIT PULL] Selection API and fixes for v3.2

[Mauro Carvalho Chehab]

Em 26-09-2011 09:10, Mauro Carvalho Chehab escreveu:
> Em 26-09-2011 05:42, Tomasz Stanislawski escreveu:
>> On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:
>>
>> Hi Mauro
>> Thank you for your comments. Please refer to the answers below.
>>> Em 22-09-2011 12:13, Marek Szyprowski escreveu:
>>>> Hello Mauro,
>>>>
>>>> I've collected pending selection API patches together with pending
>>>> videobuf2 and Samsung driver fixes to a single git branch. Please pull
>>>> them to your media tree.
>>>>
>>>
>>>> Marek Szyprowski (1):
>>>>        staging: dt3155v4l: fix build break
>>>
>>> I've applied this one previously, from the patch you sent me.
>>>
>>>
>>>> Tomasz Stanislawski (6):
>>>>        v4l: add support for selection api
>>>>        v4l: add documentation for selection API
>>>
>>> I need more time to review those two patches. I'll probably do it at the next week.
>>> I generally start analyzing API changes based on the DocBook, so, let me point a few
>>> things I've noticed on a quick read, at the vidioc-g-selection.html DocBook-generated page:
>>>
>>> 1) "The coordinates are expressed in driver-dependant units"
>>>
>>> Why? coordinates should be expressed in pixels, as otherwise there's no way to
>>> use this API on a hardware-independent way.
>> The documentation for existing cropping API contains following sentence:
>>
>> "To support a wide range of hardware this specification does not define an origin or units."
>>
>> I decided to follow the same convention for the selection API.
>> I thought that the only exception would be images in memory buffers, whose coordinated would be pixels.
>>
>> However, as Laurent mentioned, some devices are capable of sub-pixel cropping. Moreover, there are image exotic formats that have no well-defined resolution (fractal or vector graphics). 
>> Now I am still not sure if the requirement for resolution in pixels should be used any more. The problem is that this requirement is very intuitive and useful in "let's say" 95% of cases.
> 
> How an userspace application is supposed to know the type of scale?
> I can't see any way of querying the scale type.
> 
>>>
>>> 2)
>>>      0 - driver is free to adjust size, it is recommended to choose the crop/compose rectangle as close as possible to the original one
>>>
>>>      SEL_SIZE_GE - driver is not allowed to shrink the rectangle. The original rectangle must lay inside the adjusted one
>>>
>>>      SEL_SIZE_LE - drive is not allowed to grow the rectangle. The adjusted rectangle must lay inside the original one
>>>
>>>      SEL_SIZE_GE | SEL_SIZE_LE - choose size exactly the same as in desired rectangle.
>>>
>>> The macro names above don't match the definition, as they aren't prefixed by V4L2_.
>> ok.. I will fix it.
>>>
>>> 3) There was no hyperlink for the struct v4l2_selection, as on other API definitions.
>> ok.. I will fix it.
>>>
>>> 4) the language doesn't seem too consistent with the way other ioctl's are defined. For example,
>>> you're using struct::field for a field at the struct. Other parts of the API just say "field foo of struct bar".
>> ok.. I will fix it.
>>
>>> 5) There's not a single mention at the git commit or at the DocBook about why the old crop API
>>> is being deprecated. You need to convince me about such need (ok, I followed a few discussions in
>>> the past, but, my brain patch buffer is shorter than the 7000 patchwork patches I reviewed just on
>>> this week). Besides that: do we really need to obsolete the crop API for TV cards? If so, why? If not,
>>> you need to explain why a developer should opt between one ioctl set of the other.
>>
>> There are a few reasons to drop support for the cropping ioctls.
>>
>> First, the selection API covers existing crop API. Therefore there is no need to implement {S/G}_CROP and G_CROPCAP. 
>> The {S/G}_SELECTION is enough to provide the same functionality. We should avoiding duplication inside the api, 
>> therefore S_CROP should be dropped.
>>
>> Second, there is a patch that adds simulation of old crop API using selection API. Therefore there is no need to 
>> change code of the existing applications.
> 
> Both are fine, but you should notice that they aren't arguments why an
> userspace application or a driver shouldn't implement/use the crop API.
> 
>> Third, the selection is much more powerful API, and it could be extended in future. There no more reserved fields inside structures
>> used by current cropping api.
> 
> Ok.
> 
>> Moreover cropping is very inconsistent.
> 
> Why? Please describe its inconsistencies at the DocBook.
> 
>> For output devices cropping into display actually means composing into display.
> 
> Ok.
> 
>> Moreover it not possible to select only 
>> the part of the image from the buffer to be inserted or filled by the hardware. 
> 
> You're talking again on output devices, right?
> 
> 
>> The selection API introduced the idea 
>> of the constraints flags, that are used for precise control of the coordinates' rounding policy.
> 
> Ok, but I fail to see where a rounding policy would be
> needed on an input device.
> 
> In other words, I think we should split the issues with 
> the crop api into two groups:
> 
> 1) for input devices
> 2) for output devices.
> 
> Are your idea to deprecate the usage of the crop API for both
> input and output devices? 
> 
> Please correct me if I'm wrong, but the problems you've mentioned are all
> about (2), right?
> 
> The crop API were designed originally for input devices, and is currently
> used on several TV and USB webcam devices for input. 
> Grepping for vidioc_s_crap:
> 	drivers/media/video/bt8xx/bttv-driver.c
> 	drivers/media/video/et61x251/et61x251_core.c
> 	drivers/media/video/saa7134/saa7134-video.c
> 	drivers/media/video/sn9c102/sn9c102_core.c
> 	drivers/media/video/vino.c
> 	drivers/media/video/zoran/zoran_driver.c
> 	drivers/media/video/cx18/cx18-ioctl.c
> 
> Porting them to the selection API means that userspace applications
> and kernel drivers will need changes, and this will take a long time
> to happen. Ok, this can be done, if there will be large gains on it,
> but I fail to see what will be such gains.
> 
> So, please explain us why the above drivers would need to be ported to
> the selection API, or why new input drivers/applications would need
> the new API instead of the old one.
> 
> With respect to (2), the only TV device that (ab)used the crop API to 
> control its output node is
> 	drivers/media/video/ivtv/ivtv-ioctl.c
> 
> To complete the drivers list, currently, the only SoC device currently
> implementing vidioc_s_crop for input/output is the davinci driver:
> 	drivers/media/video/davinci/vpif_capture.c
> 	drivers/media/video/davinci/vpif_display.c
> 
> IMO, what it seems to be happening with the crop API is similar to what
> happened in the past with the control API: the existing API works fine
> on simple cases, but fails on more complex scenarios. In the case of
> the control API, several controls need to be grouped when selecting an
> mpeg compression parameters. So, the VIDIOC_[G|S]_EXT_CTRLS were added
> without deprecating the old ioctl's. This way, applications that were
> only supporting controls like bright, volume, etc won't need to be changed.
> 
> Internally, it made sense to implement a core way of converting a legacy
> ioctl call into the new callbacks for the _EXT_ ioctl's, and to convert
> the existing drivers.
> 
>> I could split commit 'v4l: add documentation for selection API' into two commits. One that deprecates S_CROP, and another one that introduces selection.
> 
> Actually, It is too soon to deprecate S_CROP while the selection API is tagged
> as experimental, but if this is the idea, it is better to add a hint at the V4L2
> DocBook.
> 
>>> 6) You should add a note about it at hist-v4l2.html page, stating what happened, and why a new crop
>>> ioctl set is needed.
>> ok.. I missed it. Sorry.
>>>
>>> 7) You didn't update the Experimental API Elements or the Obsolete API Elements at the hist-v4l2.html
>> ok.. I missed it. Sorry.
>>>
>>> Thanks,
>>> Mauro
>>
>> Thank again for your comments.
>> I hope that my answers will convince you to the selection API.

In time: It is not a matter of convincing me, but, instead, to convince the
developers of the above drivers and the userspace application developers why
they should care to move from the CROP API to the one you're proposing.

>>
>> Best regards,
>> Tomasz Stanislawski
>>
> 
> Regards,
> Mauro
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] Selection API and fixes for v3.2

[Hans Verkuil]

On Saturday, September 24, 2011 05:58:25 Mauro Carvalho Chehab wrote:
> Em 22-09-2011 12:13, Marek Szyprowski escreveu:
> > Hello Mauro,
> > 
> > I've collected pending selection API patches together with pending
> > videobuf2 and Samsung driver fixes to a single git branch. Please pull
> > them to your media tree.
> > 
> 
> > Marek Szyprowski (1):
> >       staging: dt3155v4l: fix build break
> 
> I've applied this one previously, from the patch you sent me.
> 
>  
> > Tomasz Stanislawski (6):
> >       v4l: add support for selection api
> >       v4l: add documentation for selection API
> 
> I need more time to review those two patches. I'll probably do it at the next week.

I also still need to review these patches, I will also be doing that this week.
Real-life interfered for the past 3-4 weeks, and I've just triaged all the
accumulated emails from that period. Jeez, linux-media is starting to resemble
linux-kernel when it comes to the volume of postings...

If this trend continues than the only way you're able to follow the traffic is
if your name is Jon Corbet :-)

Regards,

	Hans

> I generally start analyzing API changes based on the DocBook, so, let me point a few
> things I've noticed on a quick read, at the vidioc-g-selection.html DocBook-generated page:
> 
> 1) "The coordinates are expressed in driver-dependant units"
> 
> Why? coordinates should be expressed in pixels, as otherwise there's no way to
> use this API on a hardware-independent way.
> 
> 2)
>     0 - driver is free to adjust size, it is recommended to choose the crop/compose rectangle as close as possible to the original one
> 
>     SEL_SIZE_GE - driver is not allowed to shrink the rectangle. The original rectangle must lay inside the adjusted one
> 
>     SEL_SIZE_LE - drive is not allowed to grow the rectangle. The adjusted rectangle must lay inside the original one
> 
>     SEL_SIZE_GE | SEL_SIZE_LE - choose size exactly the same as in desired rectangle.
> 
> The macro names above don't match the definition, as they aren't prefixed by V4L2_.
> 
> 3) There was no hyperlink for the struct v4l2_selection, as on other API definitions.
> 
> 4) the language doesn't seem too consistent with the way other ioctl's are defined. For example,
> you're using struct::field for a field at the struct. Other parts of the API just say "field foo of struct bar".
> 
> 5) There's not a single mention at the git commit or at the DocBook about why the old crop API
> is being deprecated. You need to convince me about such need (ok, I followed a few discussions in
> the past, but, my brain patch buffer is shorter than the 7000 patchwork patches I reviewed just on
> this week). Besides that: do we really need to obsolete the crop API for TV cards? If so, why? If not,
> you need to explain why a developer should opt between one ioctl set of the other.
> 
> 6) You should add a note about it at hist-v4l2.html page, stating what happened, and why a new crop
> ioctl set is needed.
> 
> 7) You didn't update the Experimental API Elements or the Obsolete API Elements at the hist-v4l2.html
> 
> Thanks,
> Mauro
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [GIT PULL] Selection API and fixes for v3.2

[Mauro Carvalho Chehab]

Em 26-09-2011 08:21, Marek Szyprowski escreveu:
> Hello,
> 
> On Monday, September 26, 2011 1:14 PM Mauro Carvalho Chehab wrote:
> 
>>> Scott Jiang (1):
>>>       vb2: add vb2_get_unmapped_area in vb2 core
>>
>>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>>> index ea55c08..977410b 100644
>>> --- a/include/media/videobuf2-core.h
>>> +++ b/include/media/videobuf2-core.h
>>> @@ -309,6 +309,13 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type);
>>>  int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type);
>>>
>>>  int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma);
>>> +#ifndef CONFIG_MMU
>>> +unsigned long vb2_get_unmapped_area(struct vb2_queue *q,
>>> +				    unsigned long addr,
>>> +				    unsigned long len,
>>> +				    unsigned long pgoff,
>>> +				    unsigned long flags);
>>> +#endif
>>>  unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait);
>>>  size_t vb2_read(struct vb2_queue *q, char __user *data, size_t count,
>>>  		loff_t *ppos, int nonblock);
>>
>> This sounds me like a hack, as it is passing the problem of working with a non-mmu
>> capable hardware to the driver, inserting architecture-dependent bits on them.
>>
>> The proper way to do it is to provide a vb2 core support to handle the non-mmu case
>> inside it.
> 
> This is exactly what this patch does - it provides generic vb2 implementation for 
> fops->get_unmapped_area callback which any vb2 ready driver can use. This operation
> is used only on NON-MMU systems. Please check drivers/media/video/v4l2-dev.c file and
> the implementation of get_unmapped_area there. Similar code is used by uvc driver.

At least there, there is a:
#ifdef CONFIG_MMU
#define v4l2_get_unmapped_area NULL
#else
...
#endif

block, so, in thesis, a driver can be written to support both cases without inserting
#ifdefs inside it.

Ideally, I would prefer if all those iommu-specific calls would be inside the core.
A driver should not need to do anything special in order to support a different
(sub)architecture.


> 
> Best regards

Re: [GIT PULL] Selection API and fixes for v3.2

[Laurent Pinchart]

Hi Mauro,

On Monday 26 September 2011 15:30:15 Mauro Carvalho Chehab wrote:
> Em 26-09-2011 08:21, Marek Szyprowski escreveu:
> > On Monday, September 26, 2011 1:14 PM Mauro Carvalho Chehab wrote:
> >>> Scott Jiang (1):
> >>>       vb2: add vb2_get_unmapped_area in vb2 core
> >>> 
> >>> diff --git a/include/media/videobuf2-core.h
> >>> b/include/media/videobuf2-core.h index ea55c08..977410b 100644
> >>> --- a/include/media/videobuf2-core.h
> >>> +++ b/include/media/videobuf2-core.h
> >>> @@ -309,6 +309,13 @@ int vb2_streamon(struct vb2_queue *q, enum
> >>> v4l2_buf_type type);
> >>> 
> >>>  int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type);
> >>>  
> >>>  int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma);
> >>> 
> >>> +#ifndef CONFIG_MMU
> >>> +unsigned long vb2_get_unmapped_area(struct vb2_queue *q,
> >>> +				    unsigned long addr,
> >>> +				    unsigned long len,
> >>> +				    unsigned long pgoff,
> >>> +				    unsigned long flags);
> >>> +#endif
> >>> 
> >>>  unsigned int vb2_poll(struct vb2_queue *q, struct file *file,
> >>>  poll_table *wait); size_t vb2_read(struct vb2_queue *q, char __user
> >>>  *data, size_t count,
> >>>  
> >>>  		loff_t *ppos, int nonblock);
> >> 
> >> This sounds me like a hack, as it is passing the problem of working with
> >> a non-mmu capable hardware to the driver, inserting
> >> architecture-dependent bits on them.
> >> 
> >> The proper way to do it is to provide a vb2 core support to handle the
> >> non-mmu case inside it.
> > 
> > This is exactly what this patch does - it provides generic vb2
> > implementation for fops->get_unmapped_area callback which any vb2 ready
> > driver can use. This operation is used only on NON-MMU systems. Please
> > check drivers/media/video/v4l2-dev.c file and the implementation of
> > get_unmapped_area there. Similar code is used by uvc driver.
> 
> At least there, there is a:
> #ifdef CONFIG_MMU
> #define v4l2_get_unmapped_area NULL
> #else
> ...
> #endif
> 
> block, so, in thesis, a driver can be written to support both cases without
> inserting #ifdefs inside it.
> 
> Ideally, I would prefer if all those iommu-specific calls would be inside
> the core. A driver should not need to do anything special in order to
> support a different (sub)architecture.

Just for the sake of correctness, this is about no-mmu systems, not about 
iommus.

-- 
Regards,

Laurent Pinchart

RE: [GIT PULL] Selection API and fixes for v3.2

[Marek Szyprowski]

Hello,

On Monday, September 26, 2011 3:30 PM Mauro Carvalho Chehab wrote:

> Em 26-09-2011 08:21, Marek Szyprowski escreveu:
> > Hello,
> >
> > On Monday, September 26, 2011 1:14 PM Mauro Carvalho Chehab wrote:
> >
> >>> Scott Jiang (1):
> >>>       vb2: add vb2_get_unmapped_area in vb2 core
> >>
> >>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> >>> index ea55c08..977410b 100644
> >>> --- a/include/media/videobuf2-core.h
> >>> +++ b/include/media/videobuf2-core.h
> >>> @@ -309,6 +309,13 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type);
> >>>  int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type);
> >>>
> >>>  int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma);
> >>> +#ifndef CONFIG_MMU
> >>> +unsigned long vb2_get_unmapped_area(struct vb2_queue *q,
> >>> +				    unsigned long addr,
> >>> +				    unsigned long len,
> >>> +				    unsigned long pgoff,
> >>> +				    unsigned long flags);
> >>> +#endif
> >>>  unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait);
> >>>  size_t vb2_read(struct vb2_queue *q, char __user *data, size_t count,
> >>>  		loff_t *ppos, int nonblock);
> >>
> >> This sounds me like a hack, as it is passing the problem of working with a non-mmu
> >> capable hardware to the driver, inserting architecture-dependent bits on them.
> >>
> >> The proper way to do it is to provide a vb2 core support to handle the non-mmu case
> >> inside it.
> >
> > This is exactly what this patch does - it provides generic vb2 implementation for
> > fops->get_unmapped_area callback which any vb2 ready driver can use. This operation
> > is used only on NON-MMU systems. Please check drivers/media/video/v4l2-dev.c file and
> > the implementation of get_unmapped_area there. Similar code is used by uvc driver.
> 
> At least there, there is a:
> #ifdef CONFIG_MMU
> #define v4l2_get_unmapped_area NULL
> #else
> ...
> #endif
> 
> block, so, in thesis, a driver can be written to support both cases without inserting
> #ifdefs inside it.

videobuf2 functions are not meant to be used as direct callbacks in fops so defining it
as NULL makes no sense at all.

> Ideally, I would prefer if all those iommu-specific calls would be inside the core.
> A driver should not need to do anything special in order to support a different
> (sub)architecture.

It is not about IOMMU at all. Some (embedded) systems don't have MMU at all. Drivers on
such systems cannot do regular mmap operation. Instead it is emulated with 
get_unmapped_area fops callback and some (u)libC magic. This patch just provides
vb2_get_unmapped_area function. Drivers have to call it from their respective
my_driver_get_unmapped_area() function provided in its fops. Implementing it makes
sense only on NO-MMU systems. I really see no other way of dealing with this.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [GIT PULL] Selection API and fixes for v3.2

[Mauro Carvalho Chehab]

Em 27-09-2011 05:23, Marek Szyprowski escreveu:
> Hello,
> 
> On Monday, September 26, 2011 3:30 PM Mauro Carvalho Chehab wrote:
> 
>> Em 26-09-2011 08:21, Marek Szyprowski escreveu:
>>> Hello,
>>>
>>> On Monday, September 26, 2011 1:14 PM Mauro Carvalho Chehab wrote:
>>>
>>>>> Scott Jiang (1):
>>>>>       vb2: add vb2_get_unmapped_area in vb2 core
>>>>
>>>>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>>>>> index ea55c08..977410b 100644
>>>>> --- a/include/media/videobuf2-core.h
>>>>> +++ b/include/media/videobuf2-core.h
>>>>> @@ -309,6 +309,13 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type);
>>>>>  int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type);
>>>>>
>>>>>  int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma);
>>>>> +#ifndef CONFIG_MMU
>>>>> +unsigned long vb2_get_unmapped_area(struct vb2_queue *q,
>>>>> +				    unsigned long addr,
>>>>> +				    unsigned long len,
>>>>> +				    unsigned long pgoff,
>>>>> +				    unsigned long flags);
>>>>> +#endif
>>>>>  unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait);
>>>>>  size_t vb2_read(struct vb2_queue *q, char __user *data, size_t count,
>>>>>  		loff_t *ppos, int nonblock);
>>>>
>>>> This sounds me like a hack, as it is passing the problem of working with a non-mmu
>>>> capable hardware to the driver, inserting architecture-dependent bits on them.
>>>>
>>>> The proper way to do it is to provide a vb2 core support to handle the non-mmu case
>>>> inside it.
>>>
>>> This is exactly what this patch does - it provides generic vb2 implementation for
>>> fops->get_unmapped_area callback which any vb2 ready driver can use. This operation
>>> is used only on NON-MMU systems. Please check drivers/media/video/v4l2-dev.c file and
>>> the implementation of get_unmapped_area there. Similar code is used by uvc driver.
>>
>> At least there, there is a:
>> #ifdef CONFIG_MMU
>> #define v4l2_get_unmapped_area NULL
>> #else
>> ...
>> #endif
>>
>> block, so, in thesis, a driver can be written to support both cases without inserting
>> #ifdefs inside it.
> 
> videobuf2 functions are not meant to be used as direct callbacks in fops so defining it
> as NULL makes no sense at all.
> 
>> Ideally, I would prefer if all those iommu-specific calls would be inside the core.
>> A driver should not need to do anything special in order to support a different
>> (sub)architecture.
> 
> It is not about IOMMU at all. Some (embedded) systems don't have MMU at all. Drivers on
> such systems cannot do regular mmap operation. Instead it is emulated with 
> get_unmapped_area fops callback and some (u)libC magic. This patch just provides
> vb2_get_unmapped_area function. Drivers have to call it from their respective
> my_driver_get_unmapped_area() function provided in its fops. Implementing it makes
> sense only on NO-MMU systems. I really see no other way of dealing with this.

Ok. Feel free to add this patch again on a next request.

Thanks,
Mauro
> 
> Best regards

Re: [GIT PULL] Selection API and fixes for v3.2

[Tomasz Stanislawski]

Hi Mauro,

On 09/26/2011 02:10 PM, Mauro Carvalho Chehab wrote:
> Em 26-09-2011 05:42, Tomasz Stanislawski escreveu:
>> On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:
>>
>> Hi Mauro
>> Thank you for your comments. Please refer to the answers below.
>>> Em 22-09-2011 12:13, Marek Szyprowski escreveu:
>>>> Hello Mauro,
>>>>
>>>> I've collected pending selection API patches together with pending
>>>> videobuf2 and Samsung driver fixes to a single git branch. Please pull
>>>> them to your media tree.
>>>>
>>>
>>>> Marek Szyprowski (1):
>>>>         staging: dt3155v4l: fix build break
>>>
>>> I've applied this one previously, from the patch you sent me.
>>>
>>>
>>>> Tomasz Stanislawski (6):
>>>>         v4l: add support for selection api
>>>>         v4l: add documentation for selection API
>>>
>>> I need more time to review those two patches. I'll probably do it at the next week.
>>> I generally start analyzing API changes based on the DocBook, so, let me point a few
>>> things I've noticed on a quick read, at the vidioc-g-selection.html DocBook-generated page:
>>>
>>> 1) "The coordinates are expressed in driver-dependant units"
>>>
>>> Why? coordinates should be expressed in pixels, as otherwise there's no way to
>>> use this API on a hardware-independent way.
>> The documentation for existing cropping API contains following sentence:
>>
>> "To support a wide range of hardware this specification does not define an origin or units."
>>
>> I decided to follow the same convention for the selection API.
>> I thought that the only exception would be images in memory buffers, whose coordinated would be pixels.
>>
>> However, as Laurent mentioned, some devices are capable of sub-pixel cropping. Moreover, there are image exotic formats that have no well-defined resolution (fractal or vector graphics).
>> Now I am still not sure if the requirement for resolution in pixels should be used any more. The problem is that this requirement is very intuitive and useful in "let's say" 95% of cases.
>
> How an userspace application is supposed to know the type of scale?
> I can't see any way of querying the scale type.
>

What do you mean by 'scale type'? Do you mean types like 'shrink', 
'enlarge', 'no scale'?

>>>
>>> 2)
>>>       0 - driver is free to adjust size, it is recommended to choose the crop/compose rectangle as close as possible to the original one
>>>
>>>       SEL_SIZE_GE - driver is not allowed to shrink the rectangle. The original rectangle must lay inside the adjusted one
>>>
>>>       SEL_SIZE_LE - drive is not allowed to grow the rectangle. The adjusted rectangle must lay inside the original one
>>>
>>>       SEL_SIZE_GE | SEL_SIZE_LE - choose size exactly the same as in desired rectangle.
>>>
>>> The macro names above don't match the definition, as they aren't prefixed by V4L2_.
>> ok.. I will fix it.
>>>
>>> 3) There was no hyperlink for the struct v4l2_selection, as on other API definitions.
>> ok.. I will fix it.
>>>
>>> 4) the language doesn't seem too consistent with the way other ioctl's are defined. For example,
>>> you're using struct::field for a field at the struct. Other parts of the API just say "field foo of struct bar".
>> ok.. I will fix it.
>>
>>> 5) There's not a single mention at the git commit or at the DocBook about why the old crop API
>>> is being deprecated. You need to convince me about such need (ok, I followed a few discussions in
>>> the past, but, my brain patch buffer is shorter than the 7000 patchwork patches I reviewed just on
>>> this week). Besides that: do we really need to obsolete the crop API for TV cards? If so, why? If not,
>>> you need to explain why a developer should opt between one ioctl set of the other.
>>
>> There are a few reasons to drop support for the cropping ioctls.
>>
>> First, the selection API covers existing crop API. Therefore there is no need to implement {S/G}_CROP and G_CROPCAP.
>> The {S/G}_SELECTION is enough to provide the same functionality. We should avoiding duplication inside the api,
>> therefore S_CROP should be dropped.
>>
>> Second, there is a patch that adds simulation of old crop API using selection API. Therefore there is no need to
>> change code of the existing applications.
>
> Both are fine, but you should notice that they aren't arguments why an
> userspace application or a driver shouldn't implement/use the crop API.
>
>> Third, the selection is much more powerful API, and it could be extended in future. There no more reserved fields inside structures
>> used by current cropping api.
>
> Ok.
>
>> Moreover cropping is very inconsistent.
>
> Why? Please describe its inconsistencies at the DocBook.
>

Ok .. I will add a new subsection about the deficiencies of old crop API.

>> For output devices cropping into display actually means composing into display.
>
> Ok.
>
>> Moreover it not possible to select only
>> the part of the image from the buffer to be inserted or filled by the hardware.
>
> You're talking again on output devices, right?

Yes.

>
>
>> The selection API introduced the idea
>> of the constraints flags, that are used for precise control of the coordinates' rounding policy.
>
> Ok, but I fail to see where a rounding policy would be
> needed on an input device.

Please refer to following use case:
- there is a video capture hardware and a grabber application
- a face-detection is implemented in capture hardware
- the application obtains the position of the face using extended control
- application would like to grab only the face, avoiding any extra content
- the application configures cropping rectangle with V4L2_SEL_SIZE_GE 
flag to assure that no part of the face is lost

>
> In other words, I think we should split the issues with
> the crop api into two groups:
>
> 1) for input devices
> 2) for output devices.
>

No. I strongly prefer to keep consistent API for both capture and output 
devices. We should avoid adding extra ioctls and structures.
Moreover, there are mem2mem devices approaching from horizon.
They combine features of both types of queues. It would be much simpler 
for developers and application to use the same API.

> Are your idea to deprecate the usage of the crop API for both
> input and output devices?

I've given up idea of deprecating old crop API. Hopefully, the selection 
will substitute the old API in time. For now, I suggest to make the 
selection an experimental API until it becomes accepted by at least few 
drivers.

>
> Please correct me if I'm wrong, but the problems you've mentioned are all
> about (2), right?
>
> The crop API were designed originally for input devices, and is currently
> used on several TV and USB webcam devices for input.
> Grepping for vidioc_s_crap:

s/crap/crop :)

> 	drivers/media/video/bt8xx/bttv-driver.c
> 	drivers/media/video/et61x251/et61x251_core.c
> 	drivers/media/video/saa7134/saa7134-video.c
> 	drivers/media/video/sn9c102/sn9c102_core.c
> 	drivers/media/video/vino.c
> 	drivers/media/video/zoran/zoran_driver.c
> 	drivers/media/video/cx18/cx18-ioctl.c
>
> Porting them to the selection API means that userspace applications
> and kernel drivers will need changes, and this will take a long time
> to happen. Ok, this can be done, if there will be large gains on it,
> but I fail to see what will be such gains.

No changes to applications have to applied. The old crop ioctl are 
simulated by selection API inside V4L2 framework. The simulation is done 
only if s_crop or s_cropcap are NULL.

>
> So, please explain us why the above drivers would need to be ported to
> the selection API, or why new input drivers/applications would need
> the new API instead of the old one.

Some of presented hardware might be capable of composing into memory 
buffers. This functionality is not available by the current API.
The support for composing operation may justify porting the drivers to 
the selection API.

>
> With respect to (2), the only TV device that (ab)used the crop API to
> control its output node is
> 	drivers/media/video/ivtv/ivtv-ioctl.c
>
> To complete the drivers list, currently, the only SoC device currently
> implementing vidioc_s_crop for input/output is the davinci driver:
> 	drivers/media/video/davinci/vpif_capture.c
> 	drivers/media/video/davinci/vpif_display.c
>
> IMO, what it seems to be happening with the crop API is similar to what
> happened in the past with the control API: the existing API works fine
> on simple cases, but fails on more complex scenarios. In the case of
> the control API, several controls need to be grouped when selecting an
> mpeg compression parameters. So, the VIDIOC_[G|S]_EXT_CTRLS were added
> without deprecating the old ioctl's. This way, applications that were
> only supporting controls like bright, volume, etc won't need to be changed.

Refer to thread:

http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/33799/focus=34727

The concept of the simple pipeline was described there. The selection is 
an important part of this proposition. If this concept is accepted then 
'simple pipeline' becomes a new V4L2 primitive. All other more complex 
cases would be covered by the multimedia device API. The selection is 
cropping/composing API dedicated for such a pipelines because old one 
was not good enough.

I agree that most of the selection configuration could be moved to 
extended control API. By applying the same logic a few further one could 
state that the whole configuration (like VIDIOC_S_FMT, VIDIOC_S_STD, 
etc) could be moved to the extended controls. Maybe only the memory and 
streaming control ioctl would survive. I think that it would be a good 
start for V4L3 project.

>
> Internally, it made sense to implement a core way of converting a legacy
> ioctl call into the new callbacks for the _EXT_ ioctl's, and to convert
> the existing drivers.

Maybe libv4l should deal with support for deprecated APIs.

>
>> I could split commit 'v4l: add documentation for selection API' into two commits. One that deprecates S_CROP, and another one that introduces selection.
>
> Actually, It is too soon to deprecate S_CROP while the selection API is tagged
> as experimental, but if this is the idea, it is better to add a hint at the V4L2
> DocBook.

agreed

>
>>> 6) You should add a note about it at hist-v4l2.html page, stating what happened, and why a new crop
>>> ioctl set is needed.
>> ok.. I missed it. Sorry.
>>>
>>> 7) You didn't update the Experimental API Elements or the Obsolete API Elements at the hist-v4l2.html
>> ok.. I missed it. Sorry.
>>>
>>> Thanks,
>>> Mauro
>>
>> Thank again for your comments.
>> I hope that my answers will convince you to the selection API.
>>
>> Best regards,
>> Tomasz Stanislawski
>>
>
> Regards,
> Mauro

Best regards,
Tomasz Stanislawski

Re: [GIT PULL] Selection API and fixes for v3.2

[Mauro Carvalho Chehab]

Em 27-09-2011 10:02, Tomasz Stanislawski escreveu:
> Hi Mauro,
> 
> On 09/26/2011 02:10 PM, Mauro Carvalho Chehab wrote:
>> Em 26-09-2011 05:42, Tomasz Stanislawski escreveu:
>>> On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:
>>>
>>> Hi Mauro
>>> Thank you for your comments. Please refer to the answers below.
>>>> Em 22-09-2011 12:13, Marek Szyprowski escreveu:
>>>>> Hello Mauro,
>>>>>
>>>>> I've collected pending selection API patches together with pending
>>>>> videobuf2 and Samsung driver fixes to a single git branch. Please pull
>>>>> them to your media tree.
>>>>>
>>>>
>>>>> Marek Szyprowski (1):
>>>>>         staging: dt3155v4l: fix build break
>>>>
>>>> I've applied this one previously, from the patch you sent me.
>>>>
>>>>
>>>>> Tomasz Stanislawski (6):
>>>>>         v4l: add support for selection api
>>>>>         v4l: add documentation for selection API
>>>>
>>>> I need more time to review those two patches. I'll probably do it at the next week.
>>>> I generally start analyzing API changes based on the DocBook, so, let me point a few
>>>> things I've noticed on a quick read, at the vidioc-g-selection.html DocBook-generated page:
>>>>
>>>> 1) "The coordinates are expressed in driver-dependant units"
>>>>
>>>> Why? coordinates should be expressed in pixels, as otherwise there's no way to
>>>> use this API on a hardware-independent way.
>>> The documentation for existing cropping API contains following sentence:
>>>
>>> "To support a wide range of hardware this specification does not define an origin or units."
>>>
>>> I decided to follow the same convention for the selection API.
>>> I thought that the only exception would be images in memory buffers, whose coordinated would be pixels.
>>>
>>> However, as Laurent mentioned, some devices are capable of sub-pixel cropping. Moreover, there are image exotic formats that have no well-defined resolution (fractal or vector graphics).
>>> Now I am still not sure if the requirement for resolution in pixels should be used any more. The problem is that this requirement is very intuitive and useful in "let's say" 95% of cases.
>>
>> How an userspace application is supposed to know the type of scale?
>> I can't see any way of querying the scale type.
>>
> 
> What do you mean by 'scale type'? Do you mean types like 'shrink', 'enlarge', 'no scale'?

I mean: what's the scale that the application should expect for cropping? pixel, sub-mixel, percentage, etc. 

>>>>
>>>> 2)
>>>>       0 - driver is free to adjust size, it is recommended to choose the crop/compose rectangle as close as possible to the original one
>>>>
>>>>       SEL_SIZE_GE - driver is not allowed to shrink the rectangle. The original rectangle must lay inside the adjusted one
>>>>
>>>>       SEL_SIZE_LE - drive is not allowed to grow the rectangle. The adjusted rectangle must lay inside the original one
>>>>
>>>>       SEL_SIZE_GE | SEL_SIZE_LE - choose size exactly the same as in desired rectangle.
>>>>
>>>> The macro names above don't match the definition, as they aren't prefixed by V4L2_.
>>> ok.. I will fix it.
>>>>
>>>> 3) There was no hyperlink for the struct v4l2_selection, as on other API definitions.
>>> ok.. I will fix it.
>>>>
>>>> 4) the language doesn't seem too consistent with the way other ioctl's are defined. For example,
>>>> you're using struct::field for a field at the struct. Other parts of the API just say "field foo of struct bar".
>>> ok.. I will fix it.
>>>
>>>> 5) There's not a single mention at the git commit or at the DocBook about why the old crop API
>>>> is being deprecated. You need to convince me about such need (ok, I followed a few discussions in
>>>> the past, but, my brain patch buffer is shorter than the 7000 patchwork patches I reviewed just on
>>>> this week). Besides that: do we really need to obsolete the crop API for TV cards? If so, why? If not,
>>>> you need to explain why a developer should opt between one ioctl set of the other.
>>>
>>> There are a few reasons to drop support for the cropping ioctls.
>>>
>>> First, the selection API covers existing crop API. Therefore there is no need to implement {S/G}_CROP and G_CROPCAP.
>>> The {S/G}_SELECTION is enough to provide the same functionality. We should avoiding duplication inside the api,
>>> therefore S_CROP should be dropped.
>>>
>>> Second, there is a patch that adds simulation of old crop API using selection API. Therefore there is no need to
>>> change code of the existing applications.
>>
>> Both are fine, but you should notice that they aren't arguments why an
>> userspace application or a driver shouldn't implement/use the crop API.
>>
>>> Third, the selection is much more powerful API, and it could be extended in future. There no more reserved fields inside structures
>>> used by current cropping api.
>>
>> Ok.
>>
>>> Moreover cropping is very inconsistent.
>>
>> Why? Please describe its inconsistencies at the DocBook.
>>
> 
> Ok .. I will add a new subsection about the deficiencies of old crop API.
> 
>>> For output devices cropping into display actually means composing into display.
>>
>> Ok.
>>
>>> Moreover it not possible to select only
>>> the part of the image from the buffer to be inserted or filled by the hardware.
>>
>> You're talking again on output devices, right?
> 
> Yes.
> 
>>
>>
>>> The selection API introduced the idea
>>> of the constraints flags, that are used for precise control of the coordinates' rounding policy.
>>
>> Ok, but I fail to see where a rounding policy would be
>> needed on an input device.
> 
> Please refer to following use case:
> - there is a video capture hardware and a grabber application
> - a face-detection is implemented in capture hardware
> - the application obtains the position of the face using extended control
> - application would like to grab only the face, avoiding any extra content
> - the application configures cropping rectangle with V4L2_SEL_SIZE_GE flag to assure that no part of the face is lost

On all cases I can think with for input devices, the rounding policy should be GE.

>>
>> In other words, I think we should split the issues with
>> the crop api into two groups:
>>
>> 1) for input devices
>> 2) for output devices.
>>
> 
> No. I strongly prefer to keep consistent API for both capture and output devices. We should avoid adding extra ioctls and structures.

Your proposal is to add extra ioctl's and structures. I'm not saying that we should have
different API's for input and for outputs. I'm just telling that we need to analyze each
case in separate.

> Moreover, there are mem2mem devices approaching from horizon.
> They combine features of both types of queues. It would be much simpler for developers and application to use the same API.

On complex cases, like mem2mem and devices with output queues, your proposal
makes sense, but simpler devices can just use the crop API for their
input nodes.

>> Are your idea to deprecate the usage of the crop API for both
>> input and output devices?
> 
> I've given up idea of deprecating old crop API. Hopefully, the selection will substitute the old API in time.

Ok.

> For now, I suggest to make the selection an experimental API until it becomes accepted by at least few drivers.

Seems fine for me.
> 
>>
>> Please correct me if I'm wrong, but the problems you've mentioned are all
>> about (2), right?
>>
>> The crop API were designed originally for input devices, and is currently
>> used on several TV and USB webcam devices for input.
>> Grepping for vidioc_s_crap:
> 
> s/crap/crop :)

Yes, typo ;)

> 
>>     drivers/media/video/bt8xx/bttv-driver.c
>>     drivers/media/video/et61x251/et61x251_core.c
>>     drivers/media/video/saa7134/saa7134-video.c
>>     drivers/media/video/sn9c102/sn9c102_core.c
>>     drivers/media/video/vino.c
>>     drivers/media/video/zoran/zoran_driver.c
>>     drivers/media/video/cx18/cx18-ioctl.c
>>
>> Porting them to the selection API means that userspace applications
>> and kernel drivers will need changes, and this will take a long time
>> to happen. Ok, this can be done, if there will be large gains on it,
>> but I fail to see what will be such gains.
> 
> No changes to applications have to applied. The old crop ioctl are simulated by selection API inside V4L2 framework. The simulation is done only if s_crop or s_cropcap are NULL.

Deprecating an API means that it will be removed. So, drivers will need
to be ported, and also userspace applications.

>> So, please explain us why the above drivers would need to be ported to
>> the selection API, or why new input drivers/applications would need
>> the new API instead of the old one.
> 
> Some of presented hardware might be capable of composing into memory buffers. This functionality is not available by the current API.
> The support for composing operation may justify porting the drivers to the selection API.

It is doubtful that any of the above hardware would support composing. 
Maybe only cx18 might have this capability, but I don't think that the
existing devices support it anyway.

>> With respect to (2), the only TV device that (ab)used the crop API to
>> control its output node is
>>     drivers/media/video/ivtv/ivtv-ioctl.c
>>
>> To complete the drivers list, currently, the only SoC device currently
>> implementing vidioc_s_crop for input/output is the davinci driver:
>>     drivers/media/video/davinci/vpif_capture.c
>>     drivers/media/video/davinci/vpif_display.c
>>
>> IMO, what it seems to be happening with the crop API is similar to what
>> happened in the past with the control API: the existing API works fine
>> on simple cases, but fails on more complex scenarios. In the case of
>> the control API, several controls need to be grouped when selecting an
>> mpeg compression parameters. So, the VIDIOC_[G|S]_EXT_CTRLS were added
>> without deprecating the old ioctl's. This way, applications that were
>> only supporting controls like bright, volume, etc won't need to be changed.
> 
> Refer to thread:
> 
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/33799/focus=34727
> 
> The concept of the simple pipeline was described there. The selection is an important part
> of this proposition. If this concept is accepted then 'simple pipeline' becomes a new V4L2 primitive.
> All other more complex cases would be covered by the multimedia device API. The selection is
> cropping/composing API dedicated for such a pipelines because old one was not good enough.
> 
> I agree that most of the selection configuration could be moved to extended control API. 
> By applying the same logic a few further one could state that the whole configuration 
> (like VIDIOC_S_FMT, VIDIOC_S_STD, etc) could be moved to the extended controls. Maybe only 
> the memory and streaming control ioctl would survive. 

I never said that we should be using the ext control API. I'm just saying that the selection
API x crop API resembles the control API x ext control API.

In other words, for the same reason we didn't deprecate the control API when the ext control API
were added, I don't think that we should deprecate the crop API in favor of the selection API.

> I think that it would be a good start for V4L3 project.

Are you proposing that we should start a V4L3 project?????? Why?

Only this year we were able of get rid of V4L1 API, after 10+ years of efforts!
Why? Moving from V4L2 to anything else will likely take even more time, as we
now have much more drivers to take care with.

>> Internally, it made sense to implement a core way of converting a legacy
>> ioctl call into the new callbacks for the _EXT_ ioctl's, and to convert
>> the existing drivers.
> 
> Maybe libv4l should deal with support for deprecated APIs.
> 
>>
>>> I could split commit 'v4l: add documentation for selection API' into two commits. One that deprecates S_CROP, and another one that introduces selection.
>>
>> Actually, It is too soon to deprecate S_CROP while the selection API is tagged
>> as experimental, but if this is the idea, it is better to add a hint at the V4L2
>> DocBook.
> 
> agreed
> 
>>
>>>> 6) You should add a note about it at hist-v4l2.html page, stating what happened, and why a new crop
>>>> ioctl set is needed.
>>> ok.. I missed it. Sorry.
>>>>
>>>> 7) You didn't update the Experimental API Elements or the Obsolete API Elements at the hist-v4l2.html
>>> ok.. I missed it. Sorry.
>>>>
>>>> Thanks,
>>>> Mauro
>>>
>>> Thank again for your comments.
>>> I hope that my answers will convince you to the selection API.
>>>
>>> Best regards,
>>> Tomasz Stanislawski
>>>
>>
>> Regards,
>> Mauro
> 
> Best regards,
> Tomasz Stanislawski
> 
> 

Re: [GIT PULL] Selection API and fixes for v3.2

[Tomasz Stanislawski]

Hi Mauro,

On 09/27/2011 04:10 PM, Mauro Carvalho Chehab wrote:
> Em 27-09-2011 10:02, Tomasz Stanislawski escreveu:
>> On 09/26/2011 02:10 PM, Mauro Carvalho Chehab wrote:
>>> Em 26-09-2011 05:42, Tomasz Stanislawski escreveu:
>>>> On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:
>>>>> Em 22-09-2011 12:13, Marek Szyprowski escreveu:

[snip]

>>
>> What do you mean by 'scale type'? Do you mean types like 'shrink', 'enlarge', 'no scale'?
>
> I mean: what's the scale that the application should expect for cropping? pixel, sub-mixel, percentage, etc.
>

Hans suggest to use pixels as units. I bear to a very similar but 
slightly different idea that the driver should use units system that 
guarantees that no scaling is applied while the composing and cropping 
rectangle are equal. I mean rectangle's width and height.

[snip]

>>>> The selection API introduced the idea
>>>> of the constraints flags, that are used for precise control of the coordinates' rounding policy.
>>>
>>> Ok, but I fail to see where a rounding policy would be
>>> needed on an input device.
>>
>> Please refer to following use case:
>> - there is a video capture hardware and a grabber application
>> - a face-detection is implemented in capture hardware
>> - the application obtains the position of the face using extended control
>> - application would like to grab only the face, avoiding any extra content
>> - the application configures cropping rectangle with V4L2_SEL_SIZE_GE flag to assure that no part of the face is lost
>
> On all cases I can think with for input devices, the rounding policy should be GE.
>

OK. I think I know the use case for the LE flag.
Please, analyze the following scenario:
- video capture HW - TV card
- application - controls TV streaming on the framebuffer
- the image data are inserted directly into the framebuffer
- application gets information through extended controls that 
letterboxed signal is received
- application is working in full-screen mode. There must be no black 
border on the screen.
- application sets crop using the rectangle that covers whole useful 
image (without borders)
- application uses V4L2_SEL_SIZE_LE flags while setting crop rectangle 
to assure that no black border appears on the screen

>>>
>>> In other words, I think we should split the issues with
>>> the crop api into two groups:
>>>
>>> 1) for input devices
>>> 2) for output devices.
>>>
>>
>> No. I strongly prefer to keep consistent API for both capture and output devices. We should avoid adding extra ioctls and structures.
>
> Your proposal is to add extra ioctl's and structures.

 From the point of view of developers of new drivers:
- implement s_selection and g_selection to support selection API
- implement s_crop, g_crop, cropcap to support old API

 From application point of view:
- use VIDIOC_S_SELECTION, VIDIOC_G_SELECTION, struct v4l2_selection for 
selection API
- use VIDIOC_S_CROP, VIDIOC_G_CROP, VIDIOC_CROPCAP, struct v4l2_crop, 
v4l2_cropcap for old API

If new applications and drivers support only for selection API then we 
will have:
- less ioctl
- less structures
- more functionality

The legacy applications would be supported by simulation of old API 
using selection API.

I'm not saying that we should have
> different API's for input and for outputs. I'm just telling that we need to analyze each
> case in separate.
>

Please refer to summary of discussion about pipeline configuration.
The selection API seams to suit well to both types of devices.

>> Moreover, there are mem2mem devices approaching from horizon.
>> They combine features of both types of queues. It would be much simpler for developers and application to use the same API.
>
> On complex cases, like mem2mem and devices with output queues, your proposal
> makes sense, but simpler devices can just use the crop API for their
> input nodes.

Right. I agree that old API should be kept for backward compatibility 
reasons.

BTW, till now I understood that state 'deprecated' just means to avoid 
using it in the new code.

[snip]

> Deprecating an API means that it will be removed. So, drivers will need
> to be ported, and also userspace applications.
>

Ok.. I agree that it is to early to deprecate the old API.

>>> So, please explain us why the above drivers would need to be ported to
>>> the selection API, or why new input drivers/applications would need
>>> the new API instead of the old one.
>>
>> Some of presented hardware might be capable of composing into memory buffers. This functionality is not available by the current API.
>> The support for composing operation may justify porting the drivers to the selection API.
>
> It is doubtful that any of the above hardware would support composing.
> Maybe only cx18 might have this capability, but I don't think that the
> existing devices support it anyway.
>

S5P-FIMC supports this operation. OMAP3 does it too as I know.

>>> With respect to (2), the only TV device that (ab)used the crop API to
>>> control its output node is
>>>      drivers/media/video/ivtv/ivtv-ioctl.c
>>>
>>> To complete the drivers list, currently, the only SoC device currently
>>> implementing vidioc_s_crop for input/output is the davinci driver:
>>>      drivers/media/video/davinci/vpif_capture.c
>>>      drivers/media/video/davinci/vpif_display.c
>>>
>>> IMO, what it seems to be happening with the crop API is similar to what
>>> happened in the past with the control API: the existing API works fine
>>> on simple cases, but fails on more complex scenarios. In the case of
>>> the control API, several controls need to be grouped when selecting an
>>> mpeg compression parameters. So, the VIDIOC_[G|S]_EXT_CTRLS were added
>>> without deprecating the old ioctl's. This way, applications that were
>>> only supporting controls like bright, volume, etc won't need to be changed.
>>
>> Refer to thread:
>>
>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/33799/focus=34727
>>
>> The concept of the simple pipeline was described there. The selection is an important part
>> of this proposition. If this concept is accepted then 'simple pipeline' becomes a new V4L2 primitive.
>> All other more complex cases would be covered by the multimedia device API. The selection is
>> cropping/composing API dedicated for such a pipelines because old one was not good enough.
>>
>> I agree that most of the selection configuration could be moved to extended control API.
>> By applying the same logic a few further one could state that the whole configuration
>> (like VIDIOC_S_FMT, VIDIOC_S_STD, etc) could be moved to the extended controls. Maybe only
>> the memory and streaming control ioctl would survive.
>
> I never said that we should be using the ext control API. I'm just saying that the selection
> API x crop API resembles the control API x ext control API.

I agree. However these ioctls are strongly connect with memory 
management in the similar way as VIDIOC_S_FMT does. Moreover the 
selection is a simple and robust API that covers a wide range of 
build-in pipelines. Due to its generality, I think it is worth to be 
promoted with dedicated ioctl.

>
> In other words, for the same reason we didn't deprecate the control API when the ext control API
> were added, I don't think that we should deprecate the crop API in favor of the selection API.
>
>> I think that it would be a good start for V4L3 project.
>
> Are you proposing that we should start a V4L3 project?????? Why?
>
> Only this year we were able of get rid of V4L1 API, after 10+ years of efforts!
> Why? Moving from V4L2 to anything else will likely take even more time, as we
> now have much more drivers to take care with.

I don't want to start V4L3 project. I just hope that if it ever happens 
then V4L3 would a completely controls-based API with well defined 
pipeline configuration rules and memory management compatible with other 
popular APIs.

Best regards,
Tomasz Stanislawski

Re: [GIT PULL] Selection API and fixes for v3.2

[Hans Verkuil]

On Tuesday, September 27, 2011 18:46:10 Tomasz Stanislawski wrote:
> Hi Mauro,
> 
> On 09/27/2011 04:10 PM, Mauro Carvalho Chehab wrote:
> > Em 27-09-2011 10:02, Tomasz Stanislawski escreveu:
> >> On 09/26/2011 02:10 PM, Mauro Carvalho Chehab wrote:
> >>> Em 26-09-2011 05:42, Tomasz Stanislawski escreveu:
> >>>> On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:
> >>>>> Em 22-09-2011 12:13, Marek Szyprowski escreveu:
> 
> [snip]
> 
> >>
> >> What do you mean by 'scale type'? Do you mean types like 'shrink', 'enlarge', 'no scale'?
> >
> > I mean: what's the scale that the application should expect for cropping? pixel, sub-mixel, percentage, etc.
> >
> 
> Hans suggest to use pixels as units. I bear to a very similar but 
> slightly different idea that the driver should use units system that 
> guarantees that no scaling is applied while the composing and cropping 
> rectangle are equal. I mean rectangle's width and height.

As I asked before: what's the use-case of non-pixel units anyway? I haven't
seen anything yet. In addition, do you know how other hardware handles
sub-pixel cropping/composing? Do you actually know how this is done in practice?

I strongly feel that the default unit should be pixels. If you want something
else you need to request that (by perhaps setting a SUBPIXEL flag, or by having
SUBPIXEL target rectangles, or perhaps by only doing subpixel selection on the
subdev device node).

> 
> [snip]
> 
> >>>> The selection API introduced the idea
> >>>> of the constraints flags, that are used for precise control of the coordinates' rounding policy.
> >>>
> >>> Ok, but I fail to see where a rounding policy would be
> >>> needed on an input device.
> >>
> >> Please refer to following use case:
> >> - there is a video capture hardware and a grabber application
> >> - a face-detection is implemented in capture hardware
> >> - the application obtains the position of the face using extended control
> >> - application would like to grab only the face, avoiding any extra content
> >> - the application configures cropping rectangle with V4L2_SEL_SIZE_GE flag to assure that no part of the face is lost
> >
> > On all cases I can think with for input devices, the rounding policy should be GE.
> >
> 
> OK. I think I know the use case for the LE flag.
> Please, analyze the following scenario:
> - video capture HW - TV card
> - application - controls TV streaming on the framebuffer
> - the image data are inserted directly into the framebuffer
> - application gets information through extended controls that 
> letterboxed signal is received
> - application is working in full-screen mode. There must be no black 
> border on the screen.
> - application sets crop using the rectangle that covers whole useful 
> image (without borders)
> - application uses V4L2_SEL_SIZE_LE flags while setting crop rectangle 
> to assure that no black border appears on the screen
> 
> >>>
> >>> In other words, I think we should split the issues with
> >>> the crop api into two groups:
> >>>
> >>> 1) for input devices
> >>> 2) for output devices.
> >>>
> >>
> >> No. I strongly prefer to keep consistent API for both capture and output devices. We should avoid adding extra ioctls and structures.
> >
> > Your proposal is to add extra ioctl's and structures.
> 
>  From the point of view of developers of new drivers:
> - implement s_selection and g_selection to support selection API
> - implement s_crop, g_crop, cropcap to support old API
> 
>  From application point of view:
> - use VIDIOC_S_SELECTION, VIDIOC_G_SELECTION, struct v4l2_selection for 
> selection API
> - use VIDIOC_S_CROP, VIDIOC_G_CROP, VIDIOC_CROPCAP, struct v4l2_crop, 
> v4l2_cropcap for old API
> 
> If new applications and drivers support only for selection API then we 
> will have:
> - less ioctl
> - less structures
> - more functionality
> 
> The legacy applications would be supported by simulation of old API 
> using selection API.

As I said before, G/S_CROP is perfectly valid and will not go away or be
deprecated. Just as S_CTRL is not replaced by S_EXT_CTRLS. There is no need
to force apps to move to the selection API. The selection API extends the
old crop API for good reasons, but for simple cropping S_CROP remains perfectly
fine.

What would be nice is to deprecate the old crop ops for new drivers and (ideally)
convert existing drivers that use vidioc_g/s_crop to the new vidioc_g/s_selection
(with the final goal of removing vidioc_g/s_crop).

And also note that cropcap is still needed to get the pixelaspect.

> 
> I'm not saying that we should have
> > different API's for input and for outputs. I'm just telling that we need to analyze each
> > case in separate.
> >
> 
> Please refer to summary of discussion about pipeline configuration.
> The selection API seams to suit well to both types of devices.
> 
> >> Moreover, there are mem2mem devices approaching from horizon.
> >> They combine features of both types of queues. It would be much simpler for developers and application to use the same API.
> >
> > On complex cases, like mem2mem and devices with output queues, your proposal
> > makes sense, but simpler devices can just use the crop API for their
> > input nodes.
> 
> Right. I agree that old API should be kept for backward compatibility 
> reasons.
> 
> BTW, till now I understood that state 'deprecated' just means to avoid 
> using it in the new code.

And even with that meaning you still can't deprecate the application API.

> 
> [snip]
> 
> > Deprecating an API means that it will be removed. So, drivers will need
> > to be ported, and also userspace applications.
> >
> 
> Ok.. I agree that it is to early to deprecate the old API.
> 
> >>> So, please explain us why the above drivers would need to be ported to
> >>> the selection API, or why new input drivers/applications would need
> >>> the new API instead of the old one.
> >>
> >> Some of presented hardware might be capable of composing into memory buffers. This functionality is not available by the current API.
> >> The support for composing operation may justify porting the drivers to the selection API.
> >
> > It is doubtful that any of the above hardware would support composing.
> > Maybe only cx18 might have this capability, but I don't think that the

ivtv, not cx18.

> > existing devices support it anyway.

Most devices that can scale can do composition by being creative with the DMA
engine setup. Just by fiddling with strides and offsets you can achieve this.

I think you can even do this today using USERPTR and fiddling with bytesperline
in v4l2_pix_format. Most drivers will probably ignore the bytesperline value
though. And it won't work for read or memory mapped streaming.

> >
> 
> S5P-FIMC supports this operation. OMAP3 does it too as I know.
> 
> >>> With respect to (2), the only TV device that (ab)used the crop API to
> >>> control its output node is
> >>>      drivers/media/video/ivtv/ivtv-ioctl.c
> >>>
> >>> To complete the drivers list, currently, the only SoC device currently
> >>> implementing vidioc_s_crop for input/output is the davinci driver:
> >>>      drivers/media/video/davinci/vpif_capture.c
> >>>      drivers/media/video/davinci/vpif_display.c
> >>>
> >>> IMO, what it seems to be happening with the crop API is similar to what
> >>> happened in the past with the control API: the existing API works fine
> >>> on simple cases, but fails on more complex scenarios. In the case of
> >>> the control API, several controls need to be grouped when selecting an
> >>> mpeg compression parameters. So, the VIDIOC_[G|S]_EXT_CTRLS were added
> >>> without deprecating the old ioctl's. This way, applications that were
> >>> only supporting controls like bright, volume, etc won't need to be changed.
> >>
> >> Refer to thread:
> >>
> >> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/33799/focus=34727
> >>
> >> The concept of the simple pipeline was described there. The selection is an important part
> >> of this proposition. If this concept is accepted then 'simple pipeline' becomes a new V4L2 primitive.
> >> All other more complex cases would be covered by the multimedia device API. The selection is
> >> cropping/composing API dedicated for such a pipelines because old one was not good enough.
> >>
> >> I agree that most of the selection configuration could be moved to extended control API.
> >> By applying the same logic a few further one could state that the whole configuration
> >> (like VIDIOC_S_FMT, VIDIOC_S_STD, etc) could be moved to the extended controls. Maybe only
> >> the memory and streaming control ioctl would survive.
> >
> > I never said that we should be using the ext control API. I'm just saying that the selection
> > API x crop API resembles the control API x ext control API.
> 
> I agree. However these ioctls are strongly connect with memory 
> management in the similar way as VIDIOC_S_FMT does. Moreover the 
> selection is a simple and robust API that covers a wide range of 
> build-in pipelines. Due to its generality, I think it is worth to be 
> promoted with dedicated ioctl.

I've lost the plot here. This has nothing to do with pipelines, controls or V4L3.
The simple fact of the matter is that the current crop API is insufficiently
powerful (and also confusing since for outputs it does composition instead of
cropping). And the selection API was designed to overcome those limitations.

Nothing more, nothing less.

Nobody is advocating the removal of the CROP ioctls. But it would be nice from
a driver maintenance point of view if we can convert the existing drivers to use
the selection ops instead of the crop ops as much as possible. There aren't that
many that do cropping anyway. And new drivers should implement only the selection
API.

Regards,

	Hans

> 
> >
> > In other words, for the same reason we didn't deprecate the control API when the ext control API
> > were added, I don't think that we should deprecate the crop API in favor of the selection API.
> >
> >> I think that it would be a good start for V4L3 project.
> >
> > Are you proposing that we should start a V4L3 project?????? Why?
> >
> > Only this year we were able of get rid of V4L1 API, after 10+ years of efforts!
> > Why? Moving from V4L2 to anything else will likely take even more time, as we
> > now have much more drivers to take care with.
> 
> I don't want to start V4L3 project. I just hope that if it ever happens 
> then V4L3 would a completely controls-based API with well defined 
> pipeline configuration rules and memory management compatible with other 
> popular APIs.
> 
> Best regards,
> Tomasz Stanislawski
> 

Re: [GIT PULL] Selection API and fixes for v3.2

[Laurent Pinchart]

Hi Hans,

On Wednesday 28 September 2011 10:01:03 Hans Verkuil wrote:
> On Tuesday, September 27, 2011 18:46:10 Tomasz Stanislawski wrote:
> > On 09/27/2011 04:10 PM, Mauro Carvalho Chehab wrote:
> > > Em 27-09-2011 10:02, Tomasz Stanislawski escreveu:
> > >> On 09/26/2011 02:10 PM, Mauro Carvalho Chehab wrote:
> > >>> Em 26-09-2011 05:42, Tomasz Stanislawski escreveu:
> > >>>> On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:
> > >>>>> Em 22-09-2011 12:13, Marek Szyprowski escreveu:

[snip]

> > The legacy applications would be supported by simulation of old API
> > using selection API.
> 
> As I said before, G/S_CROP is perfectly valid and will not go away or be
> deprecated. Just as S_CTRL is not replaced by S_EXT_CTRLS. There is no need
> to force apps to move to the selection API. The selection API extends the
> old crop API for good reasons, but for simple cropping S_CROP remains
> perfectly fine.

Now, of course. In a couple years time, the story will likely be different, 
and we might want to deprecate the G/S_CROP API. Shouldn't this message be 
conveyed to userspace developers ? I like the idea of asking them to favor the 
selection API over the crop API for new applications.

> What would be nice is to deprecate the old crop ops for new drivers and
> (ideally) convert existing drivers that use vidioc_g/s_crop to the new
> vidioc_g/s_selection (with the final goal of removing vidioc_g/s_crop).
> 
> And also note that cropcap is still needed to get the pixelaspect.

-- 
Regards,

Laurent Pinchart

Re: [GIT PULL] Selection API and fixes for v3.2

[Hans Verkuil]

On Wednesday, September 28, 2011 10:29:37 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wednesday 28 September 2011 10:01:03 Hans Verkuil wrote:
> > On Tuesday, September 27, 2011 18:46:10 Tomasz Stanislawski wrote:
> > > On 09/27/2011 04:10 PM, Mauro Carvalho Chehab wrote:
> > > > Em 27-09-2011 10:02, Tomasz Stanislawski escreveu:
> > > >> On 09/26/2011 02:10 PM, Mauro Carvalho Chehab wrote:
> > > >>> Em 26-09-2011 05:42, Tomasz Stanislawski escreveu:
> > > >>>> On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:
> > > >>>>> Em 22-09-2011 12:13, Marek Szyprowski escreveu:
> 
> [snip]
> 
> > > The legacy applications would be supported by simulation of old API
> > > using selection API.
> > 
> > As I said before, G/S_CROP is perfectly valid and will not go away or be
> > deprecated. Just as S_CTRL is not replaced by S_EXT_CTRLS. There is no need
> > to force apps to move to the selection API. The selection API extends the
> > old crop API for good reasons, but for simple cropping S_CROP remains
> > perfectly fine.
> 
> Now, of course. In a couple years time, the story will likely be different, 
> and we might want to deprecate the G/S_CROP API. Shouldn't this message be 
> conveyed to userspace developers ? I like the idea of asking them to favor the 
> selection API over the crop API for new applications.

Why? It's like asking them not to use G_CTRL. Never going to happen.

For one thing even if the new API arrives in, say, kernel 3.2 it will take
at least 3 years before applications can even start to assume that most users
will have upgraded to a selection-aware kernel.

It's fine of course to refer to the selection API in the current crop documentation,
particularly when using the crop API for output devices (since the old API is
very confusing in that particular use-case).

But you just can't deprecate it, nor is there IMHO any reason to do so.

I'm not sure if we ever deprecated any V4L2 API in the past. If we have it
was probably for either unused or ambiguous features that apps couldn't rely
on anyway.

Regards,

	Hans

> > What would be nice is to deprecate the old crop ops for new drivers and
> > (ideally) convert existing drivers that use vidioc_g/s_crop to the new
> > vidioc_g/s_selection (with the final goal of removing vidioc_g/s_crop).
> > 
> > And also note that cropcap is still needed to get the pixelaspect.
> 
> 

Re: [GIT PULL] Selection API and fixes for v3.2

[Tomasz Stanislawski]

Hi Hans,

On 09/28/2011 10:01 AM, Hans Verkuil wrote:
> On Tuesday, September 27, 2011 18:46:10 Tomasz Stanislawski wrote:
>> On 09/27/2011 04:10 PM, Mauro Carvalho Chehab wrote:
>>> Em 27-09-2011 10:02, Tomasz Stanislawski escreveu:
>>>> On 09/26/2011 02:10 PM, Mauro Carvalho Chehab wrote:
>>>>> Em 26-09-2011 05:42, Tomasz Stanislawski escreveu:
>>>>>> On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:
>>>>>>> Em 22-09-2011 12:13, Marek Szyprowski escreveu:
>>
>> [snip]
>>
>>>>
>>>> What do you mean by 'scale type'? Do you mean types like 'shrink', 'enlarge', 'no scale'?
>>>
>>> I mean: what's the scale that the application should expect for cropping? pixel, sub-mixel, percentage, etc.
>>>
>>
>> Hans suggest to use pixels as units. I bear to a very similar but
>> slightly different idea that the driver should use units system that
>> guarantees that no scaling is applied while the composing and cropping
>> rectangle are equal. I mean rectangle's width and height.
>
> As I asked before: what's the use-case of non-pixel units anyway?

I just prefer to avoid calling this units a pixels for signals that have 
little common with pixels. The example is analog TV. There is no such a 
thing as horizontal resolution. The units are samples in that case. I 
agree that the unit should be strongly correlated with pixels on images 
in memory buffers. From application point of there is no difference if 
the units are defined as pixels or units that guarantee no scaling if 
crop and compose are equal. The numerical value are exactly the same in 
both cases. I just think that pixels sounds strange and awkward when 
taking about analog signal or sensor arrays that use abunch of R/G/B 
detectors (subpixels).

> I haven't
> seen anything yet. In addition, do you know how other hardware handles
> sub-pixel cropping/composing? Do you actually know how this is done in practice?

I think that word subpixel is a bit ambiguous. It may mean monochrome 
light emitters like ones on LCD displays. In the contexts of scaling I 
understand the subpixel as a part of the pixel (like 1/16th of pixel).

The Video Processor chip in S5P-TV supports cropping at subpixel 
resolution. The cropping is done by using polyphase filters. I don't 
know any video hardware that supports composing at resolution higher 
than pixels. 3D graphic cards can do it but they are out of V4L2 scope :).

>
> I strongly feel that the default unit should be pixels. If you want something
> else you need to request that (by perhaps setting a SUBPIXEL flag, or by having
> SUBPIXEL target rectangles, or perhaps by only doing subpixel selection on the
> subdev device node).

Extending the selection to support subpixel resolution is easy. I and 
Laurent were discussing it. The idea was to add 'denominator' field in 
v4l2_selection. The rectangle coordinates would be divided by this 
number. The denominator equal to 0 indicates that the driver does not 
support cropping/composing at precision higher than a pixel. This 
extension could be added at any time in future.

>
>>
>> [snip]
>>
>>>>>> The selection API introduced the idea
>>>>>> of the constraints flags, that are used for precise control of the coordinates' rounding policy.
>>>>>
>>>>> Ok, but I fail to see where a rounding policy would be
>>>>> needed on an input device.
>>>>
>>>> Please refer to following use case:
>>>> - there is a video capture hardware and a grabber application
>>>> - a face-detection is implemented in capture hardware
>>>> - the application obtains the position of the face using extended control
>>>> - application would like to grab only the face, avoiding any extra content
>>>> - the application configures cropping rectangle with V4L2_SEL_SIZE_GE flag to assure that no part of the face is lost
>>>
>>> On all cases I can think with for input devices, the rounding policy should be GE.
>>>
>>
>> OK. I think I know the use case for the LE flag.
>> Please, analyze the following scenario:
>> - video capture HW - TV card
>> - application - controls TV streaming on the framebuffer
>> - the image data are inserted directly into the framebuffer
>> - application gets information through extended controls that
>> letterboxed signal is received
>> - application is working in full-screen mode. There must be no black
>> border on the screen.
>> - application sets crop using the rectangle that covers whole useful
>> image (without borders)
>> - application uses V4L2_SEL_SIZE_LE flags while setting crop rectangle
>> to assure that no black border appears on the screen
>>
>>>>>
>>>>> In other words, I think we should split the issues with
>>>>> the crop api into two groups:
>>>>>
>>>>> 1) for input devices
>>>>> 2) for output devices.
>>>>>
>>>>
>>>> No. I strongly prefer to keep consistent API for both capture and output devices. We should avoid adding extra ioctls and structures.
>>>
>>> Your proposal is to add extra ioctl's and structures.
>>
>>    From the point of view of developers of new drivers:
>> - implement s_selection and g_selection to support selection API
>> - implement s_crop, g_crop, cropcap to support old API
>>
>>   From application point of view:
>> - use VIDIOC_S_SELECTION, VIDIOC_G_SELECTION, struct v4l2_selection for
>> selection API
>> - use VIDIOC_S_CROP, VIDIOC_G_CROP, VIDIOC_CROPCAP, struct v4l2_crop,
>> v4l2_cropcap for old API
>>
>> If new applications and drivers support only for selection API then we
>> will have:
>> - less ioctl
>> - less structures
>> - more functionality
>>
>> The legacy applications would be supported by simulation of old API
>> using selection API.
>
> As I said before, G/S_CROP is perfectly valid and will not go away or be
> deprecated.

Why? Deprecating means only that this ioctls should not be used in new 
code and drivers. I do not want to remove existing ioctls. Anyway, both 
APIs can coexist. So I let's keep them both. The selection may need 
improvements so it gains 'experimental' status.

 > Just as S_CTRL is not replaced by S_EXT_CTRLS. There is no need
> to force apps to move to the selection API. The selection API extends the
> old crop API for good reasons, but for simple cropping S_CROP remains perfectly
> fine.
>
> What would be nice is to deprecate the old crop ops for new drivers and (ideally)
> convert existing drivers that use vidioc_g/s_crop to the new vidioc_g/s_selection
> (with the final goal of removing vidioc_g/s_crop).

I agree.

>
> And also note that cropcap is still needed to get the pixelaspect.

The pixelaspect is another problematic issue. IMO, it does not suit to 
crop/compose operations. This parameters is strongly related to analog 
TV. Refer to the sentence from pixelaspect definition:

"When cropping coordinates refer to square pixels, the driver sets 
pixelaspect to 1/1. Other common values are 54/59 for PAL and SECAM, 
11/10 for NTSC sampled according to [[ITU BT.601]]."

It seams that this parameters is valid in contest of analog TV standards.

What is the definition of the pixelaspect for capture devices other than 
cards for analog TV?

IMO, this field should be moved to struct v4l2_standard obtained using 
VIDIOC_ENUMSTD. There are still many reserved fields there. Of course 
this field would still be present in v4l2_cropcap for compatibility reasons.

>
>>
>> I'm not saying that we should have
>>> different API's for input and for outputs. I'm just telling that we need to analyze each
>>> case in separate.
>>>
>>
>> Please refer to summary of discussion about pipeline configuration.
>> The selection API seams to suit well to both types of devices.
>>
>>>> Moreover, there are mem2mem devices approaching from horizon.
>>>> They combine features of both types of queues. It would be much simpler for developers and application to use the same API.
>>>
>>> On complex cases, like mem2mem and devices with output queues, your proposal
>>> makes sense, but simpler devices can just use the crop API for their
>>> input nodes.
>>
>> Right. I agree that old API should be kept for backward compatibility
>> reasons.
>>
>> BTW, till now I understood that state 'deprecated' just means to avoid
>> using it in the new code.
>
> And even with that meaning you still can't deprecate the application API.
>
>>
>> [snip]
>>
>>> Deprecating an API means that it will be removed. So, drivers will need
>>> to be ported, and also userspace applications.
>>>
>>
>> Ok.. I agree that it is to early to deprecate the old API.
>>
>>>>> So, please explain us why the above drivers would need to be ported to
>>>>> the selection API, or why new input drivers/applications would need
>>>>> the new API instead of the old one.
>>>>
>>>> Some of presented hardware might be capable of composing into memory buffers. This functionality is not available by the current API.
>>>> The support for composing operation may justify porting the drivers to the selection API.
>>>
>>> It is doubtful that any of the above hardware would support composing.
>>> Maybe only cx18 might have this capability, but I don't think that the
>
> ivtv, not cx18.
>
>>> existing devices support it anyway.
>
> Most devices that can scale can do composition by being creative with the DMA
> engine setup. Just by fiddling with strides and offsets you can achieve this.

I'll try to ask the maintainers to consider using selection API for 
those drivers. Maybe they could add some fixes or improvements to 
current proposition.

>
> I think you can even do this today using USERPTR and fiddling with bytesperline
> in v4l2_pix_format. Most drivers will probably ignore the bytesperline value
> though. And it won't work for read or memory mapped streaming.

Moreover, some formats lacks well-defined byteperline, even if composing 
is possible. I mean Bayer and macroblock formats.

>
>>>
>>
>> S5P-FIMC supports this operation. OMAP3 does it too as I know.
>>
>>>>> With respect to (2), the only TV device that (ab)used the crop API to
>>>>> control its output node is
>>>>>       drivers/media/video/ivtv/ivtv-ioctl.c
>>>>>
>>>>> To complete the drivers list, currently, the only SoC device currently
>>>>> implementing vidioc_s_crop for input/output is the davinci driver:
>>>>>       drivers/media/video/davinci/vpif_capture.c
>>>>>       drivers/media/video/davinci/vpif_display.c
>>>>>
>>>>> IMO, what it seems to be happening with the crop API is similar to what
>>>>> happened in the past with the control API: the existing API works fine
>>>>> on simple cases, but fails on more complex scenarios. In the case of
>>>>> the control API, several controls need to be grouped when selecting an
>>>>> mpeg compression parameters. So, the VIDIOC_[G|S]_EXT_CTRLS were added
>>>>> without deprecating the old ioctl's. This way, applications that were
>>>>> only supporting controls like bright, volume, etc won't need to be changed.
>>>>
>>>> Refer to thread:
>>>>
>>>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/33799/focus=34727
>>>>
>>>> The concept of the simple pipeline was described there. The selection is an important part
>>>> of this proposition. If this concept is accepted then 'simple pipeline' becomes a new V4L2 primitive.
>>>> All other more complex cases would be covered by the multimedia device API. The selection is
>>>> cropping/composing API dedicated for such a pipelines because old one was not good enough.
>>>>
>>>> I agree that most of the selection configuration could be moved to extended control API.
>>>> By applying the same logic a few further one could state that the whole configuration
>>>> (like VIDIOC_S_FMT, VIDIOC_S_STD, etc) could be moved to the extended controls. Maybe only
>>>> the memory and streaming control ioctl would survive.
>>>
>>> I never said that we should be using the ext control API. I'm just saying that the selection
>>> API x crop API resembles the control API x ext control API.
>>
>> I agree. However these ioctls are strongly connect with memory
>> management in the similar way as VIDIOC_S_FMT does. Moreover the
>> selection is a simple and robust API that covers a wide range of
>> build-in pipelines. Due to its generality, I think it is worth to be
>> promoted with dedicated ioctl.
>
> I've lost the plot here. This has nothing to do with pipelines, controls or V4L3.
> The simple fact of the matter is that the current crop API is insufficiently
> powerful (and also confusing since for outputs it does composition instead of
> cropping). And the selection API was designed to overcome those limitations.
>
> Nothing more, nothing less.
>
> Nobody is advocating the removal of the CROP ioctls. But it would be nice from
> a driver maintenance point of view if we can convert the existing drivers to use
> the selection ops instead of the crop ops as much as possible. There aren't that
> many that do cropping anyway. And new drivers should implement only the selection
> API.
>
> Regards,
>
> 	Hans

Regards,
Tomasz Stanislawski

>
>>
>>>
>>> In other words, for the same reason we didn't deprecate the control API when the ext control API
>>> were added, I don't think that we should deprecate the crop API in favor of the selection API.
>>>
>>>> I think that it would be a good start for V4L3 project.
>>>
>>> Are you proposing that we should start a V4L3 project?????? Why?
>>>
>>> Only this year we were able of get rid of V4L1 API, after 10+ years of efforts!
>>> Why? Moving from V4L2 to anything else will likely take even more time, as we
>>> now have much more drivers to take care with.
>>
>> I don't want to start V4L3 project. I just hope that if it ever happens
>> then V4L3 would a completely controls-based API with well defined
>> pipeline configuration rules and memory management compatible with other
>> popular APIs.
>>
>> Best regards,
>> Tomasz Stanislawski
>>

Re: [GIT PULL] Selection API and fixes for v3.2

[Mauro Carvalho Chehab]

Em 28-09-2011 06:59, Tomasz Stanislawski escreveu:
> Hi Hans,
> 
> On 09/28/2011 10:01 AM, Hans Verkuil wrote:
>> On Tuesday, September 27, 2011 18:46:10 Tomasz Stanislawski wrote:
>>> On 09/27/2011 04:10 PM, Mauro Carvalho Chehab wrote:
>>>> Em 27-09-2011 10:02, Tomasz Stanislawski escreveu:
>>>>> On 09/26/2011 02:10 PM, Mauro Carvalho Chehab wrote:
>>>>>> Em 26-09-2011 05:42, Tomasz Stanislawski escreveu:
>>>>>>> On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:
>>>>>>>> Em 22-09-2011 12:13, Marek Szyprowski escreveu:
>>>
>>> [snip]
>>>
>>>>>
>>>>> What do you mean by 'scale type'? Do you mean types like 'shrink', 'enlarge', 'no scale'?
>>>>
>>>> I mean: what's the scale that the application should expect for cropping? pixel, sub-mixel, percentage, etc.
>>>>
>>>
>>> Hans suggest to use pixels as units. I bear to a very similar but
>>> slightly different idea that the driver should use units system that
>>> guarantees that no scaling is applied while the composing and cropping
>>> rectangle are equal. I mean rectangle's width and height.
>>
>> As I asked before: what's the use-case of non-pixel units anyway?
> 
> I just prefer to avoid calling this units a pixels for signals that have little common with pixels. 
> The example is analog TV. There is no such a thing as horizontal resolution. The units are samples in 
> that case. 

When the userspace application sets the format for the analog TV, it
tells how much pixels the sampler will use. The crop API will then tell
the initial/final pixel at the horizontal resolution that will be used
when displaying the image. See? This can happen before the settings on the
memory buffers.

> I agree that the unit should be strongly correlated with pixels on images in memory buffers. 
> From application point of there is no difference if the units are defined as pixels or units that guarantee
> no scaling if crop and compose are equal. The numerical value are exactly the same in both cases. I just think 
> that pixels sounds strange and awkward when taking about analog signal or sensor arrays that use a
> bunch of R/G/B detectors (subpixels).

If the numerical value is the same, then userspace can specify it in
pixels. However, if the SELECTION api would accept anything else,
then it should have some way for userspace to get/set using a different
type of scale like subpixels and/or percentage values.

>> I haven't
>> seen anything yet. In addition, do you know how other hardware handles
>> sub-pixel cropping/composing? Do you actually know how this is done in practice?
> 
> I think that word subpixel is a bit ambiguous. It may mean monochrome light emitters
> like ones on LCD displays. In the contexts of scaling I understand the subpixel 
> as a part of the pixel (like 1/16th of pixel).
> 
> The Video Processor chip in S5P-TV supports cropping at subpixel resolution.
> The cropping is done by using polyphase filters. I don't know any video hardware 
> that supports composing at resolution higher than pixels. 3D graphic cards can do 
> it but they are out of V4L2 scope :).

If subpixel may be required in the future, the best would be to think on how
it will be implemented, in order to be sure that the current way will be compatible
with it, for example, by having a flag there (currently set to 0) that could later
be used to specify subpixels.

>>
>> I strongly feel that the default unit should be pixels. If you want something
>> else you need to request that (by perhaps setting a SUBPIXEL flag, or by having
>> SUBPIXEL target rectangles, or perhaps by only doing subpixel selection on the
>> subdev device node).
> 
> Extending the selection to support subpixel resolution is easy. I and Laurent were 
> discussing it. The idea was to add 'denominator' field in v4l2_selection. The rectangle 
> coordinates would be divided by this number. The denominator equal to 0 indicates that 
> the driver does not support cropping/composing at precision higher than a pixel. 
> This extension could be added at any time in future.

Ok. We shouldn't add it if not needed right now, but the API specs should tell that
userspace should specify the units in pixels, after setting the resolution with
VIDIOC_S_FMT, in order to be sure that the analog sampler will be properly set, on
the analog TV case.

>>> If new applications and drivers support only for selection API then we
>>> will have:
>>> - less ioctl
>>> - less structures
>>> - more functionality
>>>
>>> The legacy applications would be supported by simulation of old API
>>> using selection API.
>>
>> As I said before, G/S_CROP is perfectly valid and will not go away or be
>> deprecated.
> 
> Why? Deprecating means only that this ioctls should not be used in new code 
> and drivers. I do not want to remove existing ioctls. Anyway, both APIs can 
> coexist. So I let's keep them both. The selection may need improvements so 
> it gains 'experimental' status.

If it is experimental, it can deprecate anything, even on the above sense.

> 
>> Just as S_CTRL is not replaced by S_EXT_CTRLS. There is no need
>> to force apps to move to the selection API. The selection API extends the
>> old crop API for good reasons, but for simple cropping S_CROP remains perfectly
>> fine.
>>
>> What would be nice is to deprecate the old crop ops for new drivers and (ideally)
>> convert existing drivers that use vidioc_g/s_crop to the new vidioc_g/s_selection
>> (with the final goal of removing vidioc_g/s_crop).
> 
> I agree.

I don't. We should stop touching at the existing drivers to add things that they
don't need, just because a new SoC chip needs some neat feature.

> 
>>
>> And also note that cropcap is still needed to get the pixelaspect.
> 
> The pixelaspect is another problematic issue. IMO, it does not suit to crop/compose operations. This parameters is strongly related to analog TV. Refer to the sentence from pixelaspect definition:
> 
> "When cropping coordinates refer to square pixels, the driver sets pixelaspect to 1/1. Other common values are 54/59 for PAL and SECAM, 11/10 for NTSC sampled according to [[ITU BT.601]]."
> 
> It seams that this parameters is valid in contest of analog TV standards.
> 
> What is the definition of the pixelaspect for capture devices other than cards for analog TV?

It would be easy to change the specs wording to apply also digital TV standards.

> IMO, this field should be moved to struct v4l2_standard obtained using VIDIOC_ENUMSTD.
> There are still many reserved fields there. Of course this field would still be present 
> in v4l2_cropcap for compatibility reasons.

I agree that this would fit better at VIDIOC_ENUMSTD, but not fully convinced that
we should change it. From one side, pixelaspect has nothing to do with crop, so it
is at the wrong place, but from the other side, this would mean that the same info
would be duplicated into two different ioctls, which is messy.

>>>> It is doubtful that any of the above hardware would support composing.
>>>> Maybe only cx18 might have this capability, but I don't think that the
>>
>> ivtv, not cx18.
>>
>>>> existing devices support it anyway.
>>
>> Most devices that can scale can do composition by being creative with the DMA
>> engine setup. Just by fiddling with strides and offsets you can achieve this.

> 
> I'll try to ask the maintainers to consider using selection API for those drivers. Maybe they could add some fixes or improvements to current proposition.

Yes, this could probably be done at bttv and cx88 hardware, by changing the RISC
code. As bttv (and saa7134) supports overlay mode, perhaps the basic code is
already there. Still, I don't think that anybody is interested (or have enough
time) to change something at this part of the code.

Regards,
Mauro

Re: [GIT PULL] Selection API and fixes for v3.2

[Hans Verkuil]

I'm going to sleep on this (sub-pixel handling) a bit and get back to you on
Friday. No, that doesn't mean I'm sleeping the whole of Thursday :-), but I do
have other things on my list for tomorrow.

In the meantime, is it possible to make a new patch series incorporating all
the latest comments? (or at least those that we all agree on)

Regards,

	Hans

Re: [GIT PULL] Selection API and fixes for v3.2

[Tomasz Stanislawski]

Hi Mauro,

On 09/28/2011 12:40 PM, Mauro Carvalho Chehab wrote:
> Em 28-09-2011 06:59, Tomasz Stanislawski escreveu:
>> On 09/28/2011 10:01 AM, Hans Verkuil wrote:
>>> On Tuesday, September 27, 2011 18:46:10 Tomasz Stanislawski wrote:
>>>> On 09/27/2011 04:10 PM, Mauro Carvalho Chehab wrote:
>>>>> Em 27-09-2011 10:02, Tomasz Stanislawski escreveu:
>>>>>> On 09/26/2011 02:10 PM, Mauro Carvalho Chehab wrote:
>>>>>>> Em 26-09-2011 05:42, Tomasz Stanislawski escreveu:
>>>>>>>> On 09/24/2011 05:58 AM, Mauro Carvalho Chehab wrote:
>>>>>>>>> Em 22-09-2011 12:13, Marek Szyprowski escreveu:
>>>>
>>>> [snip]
>>>>
>>>>>>
>>>>>> What do you mean by 'scale type'? Do you mean types like 'shrink', 'enlarge', 'no scale'?
>>>>>
>>>>> I mean: what's the scale that the application should expect for cropping? pixel, sub-mixel, percentage, etc.
>>>>>
>>>>
>>>> Hans suggest to use pixels as units. I bear to a very similar but
>>>> slightly different idea that the driver should use units system that
>>>> guarantees that no scaling is applied while the composing and cropping
>>>> rectangle are equal. I mean rectangle's width and height.
>>>
>>> As I asked before: what's the use-case of non-pixel units anyway?
>>
>> I just prefer to avoid calling this units a pixels for signals that have little common with pixels.
>> The example is analog TV. There is no such a thing as horizontal resolution. The units are samples in
>> that case.
>
> When the userspace application sets the format for the analog TV, it
> tells how much pixels the sampler will use.

I assume that you mean VIDIOC_S_FMT ioctl, right?

> The crop API will then tell
> the initial/final pixel at the horizontal resolution that will be used
> when displaying the image. See? This can happen before the settings on the
> memory buffers.

Not exactly. There was a long discussion over VIDIOC_S_FMT ioctl. It was 
decided that the ioctl does too much and that it should be modified.

BTW. What are the units currently used by VIDIOC_S_CROP ?

The spec says 'To support a wide range of hardware this specification 
does not define an origin or units'. You say that it uses pixels 
(samples). Who is right? What does the protocol for cropping 
configuration look like?

>
>> I agree that the unit should be strongly correlated with pixels on images in memory buffers.
>>  From application point of there is no difference if the units are defined as pixels or units that guarantee
>> no scaling if crop and compose are equal. The numerical value are exactly the same in both cases. I just think
>> that pixels sounds strange and awkward when taking about analog signal or sensor arrays that use a
>> bunch of R/G/B detectors (subpixels).
>
> If the numerical value is the same, then userspace can specify it in
> pixels. However, if the SELECTION api would accept anything else,
> then it should have some way for userspace to get/set using a different
> type of scale like subpixels and/or percentage values.

Why? The pixel (sample) related units are ok.
Arbitrary percentage can be computed by multiplying a requested fraction 
by size of bound rectangle?

>>> I haven't
>>> seen anything yet. In addition, do you know how other hardware handles
>>> sub-pixel cropping/composing? Do you actually know how this is done in practice?
>>
>> I think that word subpixel is a bit ambiguous. It may mean monochrome light emitters
>> like ones on LCD displays. In the contexts of scaling I understand the subpixel
>> as a part of the pixel (like 1/16th of pixel).
>>
>> The Video Processor chip in S5P-TV supports cropping at subpixel resolution.
>> The cropping is done by using polyphase filters. I don't know any video hardware
>> that supports composing at resolution higher than pixels. 3D graphic cards can do
>> it but they are out of V4L2 scope :).
>
> If subpixel may be required in the future, the best would be to think on how
> it will be implemented, in order to be sure that the current way will be compatible
> with it, for example, by having a flag there (currently set to 0) that could later
> be used to specify subpixels.
>
>>>
>>> I strongly feel that the default unit should be pixels. If you want something
>>> else you need to request that (by perhaps setting a SUBPIXEL flag, or by having
>>> SUBPIXEL target rectangles, or perhaps by only doing subpixel selection on the
>>> subdev device node).
>>
>> Extending the selection to support subpixel resolution is easy. I and Laurent were
>> discussing it. The idea was to add 'denominator' field in v4l2_selection. The rectangle
>> coordinates would be divided by this number. The denominator equal to 0 indicates that
>> the driver does not support cropping/composing at precision higher than a pixel.
>> This extension could be added at any time in future.
>
> Ok. We shouldn't add it if not needed right now, but the API specs should tell that
> userspace should specify the units in pixels, after setting the resolution with
> VIDIOC_S_FMT, in order to be sure that the analog sampler will be properly set, on
> the analog TV case.
>
>>>> If new applications and drivers support only for selection API then we
>>>> will have:
>>>> - less ioctl
>>>> - less structures
>>>> - more functionality
>>>>
>>>> The legacy applications would be supported by simulation of old API
>>>> using selection API.
>>>
>>> As I said before, G/S_CROP is perfectly valid and will not go away or be
>>> deprecated.
>>
>> Why? Deprecating means only that this ioctls should not be used in new code
>> and drivers. I do not want to remove existing ioctls. Anyway, both APIs can
>> coexist. So I let's keep them both. The selection may need improvements so
>> it gains 'experimental' status.
>
> If it is experimental, it can deprecate anything, even on the above sense.
>
OK.
>>
>>> Just as S_CTRL is not replaced by S_EXT_CTRLS. There is no need
>>> to force apps to move to the selection API. The selection API extends the
>>> old crop API for good reasons, but for simple cropping S_CROP remains perfectly
>>> fine.
>>>
>>> What would be nice is to deprecate the old crop ops for new drivers and (ideally)
>>> convert existing drivers that use vidioc_g/s_crop to the new vidioc_g/s_selection
>>> (with the final goal of removing vidioc_g/s_crop).
>>
>> I agree.
>
> I don't. We should stop touching at the existing drivers to add things that they
> don't need, just because a new SoC chip needs some neat feature.
>

We should focus on the new drivers that do not exist yet or are in 
experimental phase. The decision whether or not to implement the 
selection in the existing drivers belongs to their maintainers.

>>
>>>
>>> And also note that cropcap is still needed to get the pixelaspect.
>>
>> The pixelaspect is another problematic issue. IMO, it does not suit to crop/compose operations. This parameters is strongly related to analog TV. Refer to the sentence from pixelaspect definition:
>>
>> "When cropping coordinates refer to square pixels, the driver sets pixelaspect to 1/1. Other common values are 54/59 for PAL and SECAM, 11/10 for NTSC sampled according to [[ITU BT.601]]."
>>
>> It seams that this parameters is valid in contest of analog TV standards.
>>
>> What is the definition of the pixelaspect for capture devices other than cards for analog TV?
>
> It would be easy to change the specs wording to apply also digital TV standards.
>
>> IMO, this field should be moved to struct v4l2_standard obtained using VIDIOC_ENUMSTD.
>> There are still many reserved fields there. Of course this field would still be present
>> in v4l2_cropcap for compatibility reasons.
>
> I agree that this would fit better at VIDIOC_ENUMSTD, but not fully convinced that
> we should change it. From one side, pixelaspect has nothing to do with crop, so it
> is at the wrong place, but from the other side, this would mean that the same info
> would be duplicated into two different ioctls, which is messy.

I think it is worth to put it in two places. The pixelaspect is 
read-only value so there are no dependency problems. Another thing is 
that only analog TV drivers uses the value other than 1/1. Some drivers 
even use value 0/0.

Moreover, there are duplications in existing API. Example:
Structure v4l2_buffer has 'memory' field. The value of the field must be 
equal to the one used in VIDIOC_REQBUFS. So there is no need to use this 
field.

Best regards,
Tomasz Stanislawski

>
>>>>> It is doubtful that any of the above hardware would support composing.
>>>>> Maybe only cx18 might have this capability, but I don't think that the
>>>
>>> ivtv, not cx18.
>>>
>>>>> existing devices support it anyway.
>>>
>>> Most devices that can scale can do composition by being creative with the DMA
>>> engine setup. Just by fiddling with strides and offsets you can achieve this.
>
>>
>> I'll try to ask the maintainers to consider using selection API for those drivers. Maybe they could add some fixes or improvements to current proposition.
>
> Yes, this could probably be done at bttv and cx88 hardware, by changing the RISC
> code. As bttv (and saa7134) supports overlay mode, perhaps the basic code is
> already there. Still, I don't think that anybody is interested (or have enough
> time) to change something at this part of the code.
>
> Regards,
> Mauro