After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 742917 - v4l2: Allow scaling in the v4l2*convert element
v4l2: Allow scaling in the v4l2*convert element
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal enhancement
: 1.5.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 726104 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-01-14 13:49 UTC by Enrico Jorns
Modified: 2015-06-09 22:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
scaling patch (18.21 KB, patch)
2015-01-14 13:49 UTC, Enrico Jorns
none Details | Review
scaling patch v2 (18.55 KB, patch)
2015-04-14 07:34 UTC, Enrico Jorns
committed Details | Review

Description Enrico Jorns 2015-01-14 13:49:18 UTC
Created attachment 294525 [details] [review]
scaling patch

* Remove also 'width' and 'height' caps in gst_v4l2_transform_caps_remove_format_info()
in order to allow scaling by the underlying v4l2 element.

* Choose appropriate width and height values in caps fixation
Copied caps fixation code from gstvideoconvert element

Example usage:
[..] ! video/x-raw,width=320 ! v4l2video1convert ! video/x-raw,width=1280 ! [..]
Comment 1 Nicolas Dufresne (ndufresne) 2015-01-14 14:20:14 UTC
*** Bug 726104 has been marked as a duplicate of this bug. ***
Comment 2 Nicolas Dufresne (ndufresne) 2015-01-14 14:28:22 UTC
Thanks Enrico for this patch, I'll have a look as soon as possible. Meanwhile, I have few general question:

Is this negotiation code inspired from videoscale element ?
Have you taken pixel aspect ratio into account ?
Do you support device that cannot scale ?

Finally, just mentioning, I had a discussion with Hans and could not conclude if my use of the CROP ioctl was right or not. Ideally we'd be using SELECTION but it was not implemented when I did that work. Have you made some tests with various cropping ?

I also notice that you forgot to update the Klass field of this element. The software videoscaler sets:

Klass                    Filter/Converter/Video/Scaler

These will eventually be used for auto-plugging elements in the future.
Comment 3 Nicolas Dufresne (ndufresne) 2015-01-14 14:29:42 UTC
(oh, and I'll do some tests with FIMC driver on Exynos 4)
Comment 4 Aurélien Zanelli 2015-01-14 15:12:11 UTC
Interesting, I think I can also do some tests with my HW.

By the way, do we have a way to know that device scale other than probing method (like in https://bugzilla.gnome.org/show_bug.cgi?id=727483) ?
Comment 5 Nicolas Dufresne (ndufresne) 2015-02-25 19:40:49 UTC
(In reply to Nicolas Dufresne (stormer) from comment #2)
> 
> Is this negotiation code inspired from videoscale element ?
> Have you taken pixel aspect ratio into account ?

Answer is yes, twice.

> Do you support device that cannot scale ?
> Klass                    Filter/Converter/Video/Scaler

Any update on these ?
Comment 6 Aurélien Zanelli 2015-02-26 14:22:10 UTC
I apply the patch and it seems to work well.

(In reply to Nicolas Dufresne (stormer) from comment #5)
> > Do you support device that cannot scale ?
> > Klass                    Filter/Converter/Video/Scaler
> 
> Any update on these ?

I quckly looked at the code and I didn't find such check, so I think the answer is no. However the S_FMT should fail (hopefully).
Comment 7 Nicolas Dufresne (ndufresne) 2015-02-26 14:31:33 UTC
The problem is that we need to only suggest things that are possible in our transform caps call. Failing trying to do something that is not supported is not acceptable. There could be an element downstream the do scale correctly.

If V4L2 don't tell us that, we should at least create a white list.
Comment 8 Aurélien Zanelli 2015-02-26 14:58:27 UTC
(In reply to Nicolas Dufresne (stormer) from comment #7)
> The problem is that we need to only suggest things that are possible in our
> transform caps call. Failing trying to do something that is not supported is
> not acceptable. There could be an element downstream the do scale correctly.

I'm entirely agree with that.

> If V4L2 don't tell us that, we should at least create a white list.
There is currently a way to probe if the device can scale or not by checking the crop window against the set format. But I don't really know if it's reliable or not.
Comment 9 Nicolas Dufresne (ndufresne) 2015-02-26 18:09:28 UTC
(In reply to Aurélien Zanelli from comment #8)
> > If V4L2 don't tell us that, we should at least create a white list.
> There is currently a way to probe if the device can scale or not by checking
> the crop window against the set format. But I don't really know if it's
> reliable or not.

Oh, and I saw you patches btw, but could not completly make my mind. The crop API being what it is. (and v4l2 people telling me to use SELECTION which they didn't implement ;-P). I guess we need to do some more testing to find the right information.
Comment 10 Enrico Jorns 2015-04-14 07:33:04 UTC
(In reply to Nicolas Dufresne (stormer) from comment #7)
> The problem is that we need to only suggest things that are possible in our
> transform caps call. Failing trying to do something that is not supported is
> not acceptable. There could be an element downstream the do scale correctly.
> 
> If V4L2 don't tell us that, we should at least create a white list.

Yes, afaik there is no straight way to test if a v4l2 m2m device supports scaling. But this is the same for color conversion capability. Thus if we had a device that could scale but not convert color space, the current approach might fail, too.
In the current kernel there should be no driver that supports colorspace conversion only. So imho this patch should not make things worse.

But yes, in general this is a problem to be solved.

I've attached a modified version of the patch that adds the "Scalar" hint.
Comment 11 Enrico Jorns 2015-04-14 07:34:37 UTC
Created attachment 301509 [details] [review]
scaling patch v2
Comment 12 Enrico Jorns 2015-04-27 10:17:29 UTC
Any comments for this?
Comment 13 Nicolas Dufresne (ndufresne) 2015-04-27 12:50:28 UTC
I was on holiday. I accept the argument in comment 10. My only concern was around the granularity of the scaler and how do we handle it. Scaler often only support sizes with multiple of 2 or even 16. Also that concern that it may fail randomly since the kernel fixes to round S_FMT properly is still not merged yet.

Would it be possible to document you approach to handle a scaler that can only scale with multiple of 2 or 4 ?

Though, in general, as long as we have a plan on how we want to handle this, and that the code mostly reflect that, I'm fine.
Comment 14 Nicolas Dufresne (ndufresne) 2015-06-09 22:40:37 UTC
Review of attachment 301509 [details] [review]:

Looks good to me. I didn't tested myself though.
Comment 15 Nicolas Dufresne (ndufresne) 2015-06-09 22:44:53 UTC
Answering myself:

(In reply to Nicolas Dufresne (stormer) from comment #13)
> I was on holiday. I accept the argument in comment 10. My only concern was
> around the granularity of the scaler and how do we handle it. Scaler often
> only support sizes with multiple of 2 or even 16. Also that concern that it
> may fail randomly since the kernel fixes to round S_FMT properly is still
> not merged yet.
> 
> Would it be possible to document you approach to handle a scaler that can
> only scale with multiple of 2 or 4 ?

To work properly in generic environment, the drive should implement enumeration of formats. In that context, it will be able to use the step and range. The rest will all be handled correctly automatically.
Comment 16 Nicolas Dufresne (ndufresne) 2015-06-09 22:51:49 UTC
commit 3067a60d8e76062f42f1b605393ab50be8c9d907