I'm working on this. Will submit the patch for review once completed.

Created attachment 289514 [details] [review]
use videoconvert API for format conversion

Patch attached for proposed changes.

Few observations:
Creating a converter requires height and width of both input and output frames to be equal.
Hence an intermediate frame to be created with height and width = that of output frame.
copy the contents from input frame to intermediate frame considering border area/crop area.
Then, call videoconvert API to convert intermediate frame to output frame of desired format.

This adds some overhead. For example, when I measured the time consumed,
conversion of AYUV to I420 of size 320x240 with border -10 each side, took ~988.6ms (against ~629ms earlier)

However, when input format = output format, this overhead is avoided.
AYUV to AYUV of size 320x240 with border -10 each side, took ~161.9ms (against ~288.5ms earlier)
I420 to I420 of size 320x240 with border -10 each side, took ~80.5ms (against ~839.8ms earlier)
[the saving here is mostly due to memcpy of lines instead of pixel-by-pixel copy]

As most of the videobox usage I came across were mostly about only adding borders or cropping, there will be no overhead in those cases with the new approach.

Feedback welcome.

You can now use the cropping support in the converter directly. Could you try to update the patch? I think you can remove some more lines :)

Wim, using video-converter still crops the right side only (instead of left).
For example, I ran the pipeline 
gst-launch-1.0 videotestsrc ! "video/x-raw,format={RGB},height=480,width=640" ! videobox left=50 right=-50 ! videoconvert ! ximagesink
This should have cropped left region. But, it cropped the right side.

Part of the problem is because "unpack_RGB()" in video-format.c doesn't consider 'x' offset (actually, none of unpack functions do).
And, pack functions do not have 'x' offset parameter. 
https://bugzilla.gnome.org/show_bug.cgi?id=740228 is reported for this.

Once pack functions support 'x' parameter, I think we need to add 'x' to PACK_FRAME macro in video-converter.c

(In reply to comment #4)
> Wim, using video-converter still crops the right side only (instead of left).
> For example, I ran the pipeline 
> gst-launch-1.0 videotestsrc ! "video/x-raw,format={RGB},height=480,width=640" !
> videobox left=50 right=-50 ! videoconvert ! ximagesink
> This should have cropped left region. But, it cropped the right side.

Works for me, what are you doing? Are you using this patch? I don't see you setting any offset in the options?

> 
> Part of the problem is because "unpack_RGB()" in video-format.c doesn't
> consider 'x' offset (actually, none of unpack functions do).
> And, pack functions do not have 'x' offset parameter. 
> https://bugzilla.gnome.org/show_bug.cgi?id=740228 is reported for this.

yes, we need to add support for unpacking from a different offset.

> 
> Once pack functions support 'x' parameter, I think we need to add 'x' to
> PACK_FRAME macro in video-converter.c

no, pack_frame does not need an x parameter, only unpack.

Created attachment 291621 [details]
screenshots of output

I used latest video-converter code. 
In videobox, while creating new video-converter, I used the configuration structure, with values for
GST_VIDEO_CONVERTER_OPT_SRC_X = src_x, GST_VIDEO_CONVERTER_OPT_SRC_Y = src_y,
GST_VIDEO_CONVERTER_OPT_DEST_X = dest_x, GST_VIDEO_CONVERTER_OPT_DEST_Y = dest_y,
GST_VIDEO_CONVERTER_OPT_SRC_WIDTH = GST_VIDEO_CONVERTER_OPT_DEST_WIDTH = crop_w
GST_VIDEO_CONVERTER_OPT_SRC_HEIGHT = GST_VIDEO_CONVERTER_OPT_DEST_HEIGHT = crop_h

(Let me know if it is incorrect)

src_x, src_y, dest_x, dest_y, crop_w, crop_h are calculated as in the attached patch.
For the above pipeline, the values are - src_x = 50, src_y = 0, dest_x = 0, dest_y = 0, crop_w = 590, crop_h = 480.
In this case it cropped the right side (attached pic VB_VC.png). Output of original videobox code for same pipeline - VB_O.png

Since unpack_rgb doesn't consider 'x', it unpacks from beginning where as it should have unpacked from pos 50.
When I added the 'x' offset for unpacking, it cropped the left side correctly. 
However during packing, it left a border at the beginning. (Pic - VB_VC1.png) 

Then I tried a hack - added 'x' offset to src while 'packing'. (Pic - VB_VC2.png)

Let me know if I got something wrong. Thanks.

Created attachment 297974 [details] [review]
use video-converter APIs in videobox for format conversions

All dependencies sorted. New patch attached. Please review.
Only issue is, sometimes ARGB -> BGRx conversion crashes. I have created bug for that - https://bugzilla.gnome.org/show_bug.cgi?id=745207

Created attachment 298602 [details] [review]
use videoconvert API for format conversion

Little correction to previous patch

Seems to work for me. I noticed the old videobox can do subsample cropping for I420, video-converter can't do that yet.

Ok. Do you plan to do it in video-converter?

(In reply to RaviKiran from comment #10)
> Ok. Do you plan to do it in video-converter?

For now it can only do cropping or add a border with a width/height that is a multiple of the subsampling.

The way it was done before is actually not correct anyway, it causes a chroma shift. If we want to implement this correctly we have to move the chroma with a half pixel when we crop the luma. Not sure it is worth it for now.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/129.