[Bug 768248] New: vaapiencode: Add Encoding region-of-interest (ROI) support

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Bug ID: 768248
Summary: vaapiencode: Add Encoding region-of-interest (ROI)
support
Classification: Platform
Product: GStreamer
Version: unspecified
OS: Linux
Status: NEW
Severity: enhancement
Priority: Normal
Component: gstreamer-vaapi
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@gmail.com
QA Contact: gstreamer-***@lists.freedesktop.org
CC: ***@gmail.com, ***@igalia.com
GNOME version: ---

Add support for QP adjustments in specific(user provided) areas of the frames
when encoding.

ROIs can be set through the VAENCMiscParameterBufferROIs. The ROI set through
this structure is applicable only to the current frame/field, so must be sent
every frame/field to be applied. The number of supported ROIs can be queried
through the VAConfigAttribEncROI. The encoder will use the ROI information to
adjust the QP values of the MB's that fall within the ROIs.

This feature is for giving more control to the user in order to tune the encode
quality.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Lim Siew Hoon <***@intel.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@intel.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Hyunjun Ko <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@igalia.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #1 from Hyunjun Ko <***@igalia.com> ---
I've been working for this issue and I found some issues to discuss.

1. Handling region of interest meta?
There's already video meta for this, called "GstVideoRegionOfInterestMeta"
But, afaik, there's no element to use this except for opencv element.(face
detection)
Does vaapi encoder need to handle this or just create property like roi-x,
roi-y, roi-w, roi-h? Or both?

2. Other property to be created in vaapi encoder.
Maybe these 2 properties are necessary to provide.
- enable-roi : Enable/Disable roi encoding.
- roi-value : a kind of level of roi region
http://01org.github.io/libva_staging_doxygen/struct__VAEncMiscParameterBufferRoi_1_1VAEncROI.html#ab37eca45463295659565a46430b0b925

And not sure if these attributes are necessary or not as a property.
- max/min delta qp : works only if CQP mode
- num_of_roi : if we supports this, we should support for setting of the number
of {x,y,w,h}, corresponding to num_of_roi. but how?

Refer to
http://01org.github.io/libva_staging_doxygen/struct__VAEncMiscParameterBufferRoi.html


I think I'm going to start implementation of kinda version 0.1, but need
opinions about this :)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #2 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
From my point of view, the "gst" way of implementing this, is creating a new
GST_EVENT_CUSTOM_DOWNSTREAM (perhaps also upstream) that the application could
inject into the pipeline with gst_element_send_event().

The event will have an GstStructure named, for example
GstVaapiEncoderRegionOfInterest, with roi-x, roi-y, roi-width, roi-height, etc.

The vaapi encoders will catch this event and will configure themselves
accordingly.

That's it, from the top of my head.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Julien Isorce <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #3 from Hyunjun Ko <***@igalia.com> ---
Created attachment 344625
--> https://bugzilla.gnome.org/attachment.cgi?id=344625&action=edit
libs: encoder/context: query region of interest support

Query Region of Interest support during config creation.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #4 from Hyunjun Ko <***@igalia.com> ---
Created attachment 344626
--> https://bugzilla.gnome.org/attachment.cgi?id=344626&action=edit
libs: encoder: add api gst_vaapi_encoder_set_roi

Implements and exposes new api gst_vaapi_encoder_set_roi to set ROI regions.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #5 from Hyunjun Ko <***@igalia.com> ---
Created attachment 344627
--> https://bugzilla.gnome.org/attachment.cgi?id=344627&action=edit
libs: encoder: h264: set ROI params during encoding

Set ROI params during encoding each frame,
which is set via gst_vaapi_encoder_set_roi

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #6 from Hyunjun Ko <***@igalia.com> ---
Created attachment 344628
--> https://bugzilla.gnome.org/attachment.cgi?id=344628&action=edit
tests: simple-encoder: add an option to set ROI

Adds an option "roi" to test roi support of encoder.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #7 from Hyunjun Ko <***@igalia.com> ---
Internally, query/set ROI params are implemented in these patches.
Once these can be acceptable, then we can discuss how to provide the way to set
ROI params to users. (GstEvent, or anytihng else)

And we can go further to support for other codecs such as vp8.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #8 from Hyunjun Ko <***@igalia.com> ---
Ping.
Victor, Sree? :)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #344625|none |needs-work
status| |

--- Comment #9 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 344625:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=344625)

::: gst-libs/gst/vaapi/gstvaapicontext.c
@@ +314,3 @@
+ GST_ERROR ("ROI unsupported - number of regions supported: %d"
+ " ROI delta QP: %d", roi_config->bits.num_roi_regions,
+ roi_config->bits.roi_rc_qp_delat_support);

I guess you should improve the error message, imo it is misleading because ROI
can be supported but not the number of requested regions. Also, I would keep
this as a warning and do not break the encoding.

::: gst-libs/gst/vaapi/gstvaapiencoder.c
@@ +573,3 @@
+ GST_INFO ("ROI unsupported - number of regions supported: %d"
+ " ROI delta QP: %d", roi_config->bits.num_roi_regions,
+ roi_config->bits.roi_rc_qp_delat_support);

If ROI is unsupported, there is no reason to print the number of regions or if
delta QP is supported, since any of they are zero.

By the way, I guess we should open a bug in libva because of the typo delat ==
delta, but that will break the API.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #344626|none |needs-work
status| |

--- Comment #10 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 344626:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=344626)

::: gst-libs/gst/vaapi/gstvaapiencoder.c
@@ +1230,3 @@
+
+ if (!config->roi_capability)
+ return;

Turn the function into boolean to report back if it's possible to execute the
operation.

@@ +1240,3 @@
+ g_array_free (encoder->roi_regions, FALSE);
+
+ encoder->roi_regions = g_array_new (FALSE, FALSE, sizeof (GstVaapiROI));

As we know the size, we should use g_array_sized_new ()

::: gst-libs/gst/vaapi/gstvaapiencoder.h
@@ +129,3 @@
+ guint w;
+ guint h;
+} GstVaapiROI;

I would reuse the GstVaapiRectangle nested in GstVaapiROI

@@ +187,3 @@

+void
+gst_vaapi_encoder_set_roi (GstVaapiEncoder * encoder, GArray * roi_regions);

What about an API to add and delete single ROIs??

gboolean gst_vaapi_encoder_add_roi (GstVaapiEncoder * encoder, GstVaapiROI *
roi);
gboolean gst_vaapi_encoder_del_roi (GstVaapiEncoder * encoder, GstVaapiROI *
roi);

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #344627|none |reviewed
status| |

--- Comment #11 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 344627:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=344627)

lgtm

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #344628|none |reviewed
status| |

--- Comment #12 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 344628:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=344628)

lgtm

::: tests/simple-encoder.c
@@ +197,3 @@
+ }
+
+ gst_vaapi_encoder_set_roi (encoder, roi_regions);

Looking at this, I guess that the approach of {add,del}_roi is the way to go.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #13 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 344626:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=344626)

::: gst-libs/gst/vaapi/gstvaapiencoder.c
@@ +1227,3 @@
+ guint roi_num, i;
+
+ g_return_if_fail (roi_regions != NULL);

Is is possible to add/delete ROIs when the encoder is already processing a
stream?

If not, we should also rely on the queued_codedbuf_num

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #14 from Hyunjun Ko <***@igalia.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #9)

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/vaapi/gstvaapicontext.c
@@ +314,3 @@
+ GST_ERROR ("ROI unsupported - number of regions supported: %d"
+ " ROI delta QP: %d", roi_config->bits.num_roi_regions,
+ roi_config->bits.roi_rc_qp_delat_support);
I guess you should improve the error message, imo it is misleading because
ROI can be supported but not the number of requested regions. Also, I would
keep this as a warning and do not break the encoding.

I think this should be error since the supported roi thing is already queried
and set to config(cip->config.encoder). That's why roi supported_num should be
same. If we want to update, we should turn off "static" for
cip->config.encoder, but current logic isn't aimed at it, I guess.

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/vaapi/gstvaapiencoder.c
@@ +573,3 @@
+ GST_INFO ("ROI unsupported - number of regions supported: %d"
+ " ROI delta QP: %d", roi_config->bits.num_roi_regions,
+ roi_config->bits.roi_rc_qp_delat_support);
If ROI is unsupported, there is no reason to print the number of regions or
if delta QP is supported, since any of they are zero.

Agree, I'll change it.

Post by "GStreamer" (GNOME Bugzilla)
By the way, I guess we should open a bug in libva because of the typo delat
== delta, but that will break the API.

Wow, nice catch. I'm going to raise this issue.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #15 from Hyunjun Ko <***@igalia.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #10)

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/vaapi/gstvaapiencoder.c
@@ +1230,3 @@
+
+ if (!config->roi_capability)
+ return;
Turn the function into boolean to report back if it's possible to execute
the operation.
@@ +1240,3 @@
+ g_array_free (encoder->roi_regions, FALSE);
+
+ encoder->roi_regions = g_array_new (FALSE, FALSE, sizeof (GstVaapiROI));
As we know the size, we should use g_array_sized_new ()
::: gst-libs/gst/vaapi/gstvaapiencoder.h
@@ +129,3 @@
+ guint w;
+ guint h;
+} GstVaapiROI;
I would reuse the GstVaapiRectangle nested in GstVaapiROI
@@ +187,3 @@
+void
+gst_vaapi_encoder_set_roi (GstVaapiEncoder * encoder, GArray * roi_regions);
What about an API to add and delete single ROIs??
gboolean gst_vaapi_encoder_add_roi (GstVaapiEncoder * encoder, GstVaapiROI *
roi);
gboolean gst_vaapi_encoder_del_roi (GstVaapiEncoder * encoder, GstVaapiROI *
roi);

Well, I like your approach better than mine.
Let me finish this work according to your suggestion.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #16 from Hyunjun Ko <***@igalia.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #13)

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/vaapi/gstvaapiencoder.c
@@ +1227,3 @@
+ guint roi_num, i;
+
+ g_return_if_fail (roi_regions != NULL);
Is is possible to add/delete ROIs when the encoder is already processing a
stream?
If not, we should also rely on the queued_codedbuf_num

Yes, it's possible theoretically though I haven't tested it.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Hyunjun Ko <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #344625|needs-work |none
status| |
Attachment #344625|0 |1
is obsolete| |
Attachment #344626|needs-work |none
status| |
Attachment #344626|0 |1
is obsolete| |
Attachment #344627|reviewed |none
status| |
Attachment #344627|0 |1
is obsolete| |
Attachment #344628|reviewed |none
status| |
Attachment #344628|0 |1
is obsolete| |

--- Comment #17 from Hyunjun Ko <***@igalia.com> ---
Created attachment 346550
--> https://bugzilla.gnome.org/attachment.cgi?id=346550&action=edit
libs: encoder/context: query region of interest support

Query Region of Interest support during config creation.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #18 from Hyunjun Ko <***@igalia.com> ---
Created attachment 346551
--> https://bugzilla.gnome.org/attachment.cgi?id=346551&action=edit
libs: encoder: add api gst_vaapi_encoder_add/del_roi

Implements and exposes new api gst_vaapi_encoder_add/del_roi to set ROI
regions.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #19 from Hyunjun Ko <***@igalia.com> ---
Created attachment 346552
--> https://bugzilla.gnome.org/attachment.cgi?id=346552&action=edit
libs: encoder: h264: set ROI params during encoding

Set ROI params during encoding each frame, which is set via
gst_vaapi_encoder_add_roi

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #20 from Hyunjun Ko <***@igalia.com> ---
Created attachment 346553
--> https://bugzilla.gnome.org/attachment.cgi?id=346553&action=edit
tests: simple-encoder: add an option to set ROI

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346551|none |needs-work
status| |

--- Comment #21 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 346551:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=346551)

::: gst-libs/gst/vaapi/gstvaapiencoder.c
@@ +1211,3 @@
}

+#if 0

Why is this code disabled? Should it be removed?

@@ +1243,3 @@
+#else
+gboolean
+gst_vaapi_encoder_add_roi (GstVaapiEncoder * encoder, GstVaapiROI * roi)

Document this function. Especially if @roi transfers its ownership.

@@ +1263,3 @@
+
+gboolean
+gst_vaapi_encoder_del_roi (GstVaapiEncoder * encoder, GstVaapiROI * roi)

ditto.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346550|none |reviewed
status| |

--- Comment #22 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 346550:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=346550)

lgtm

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346552|none |reviewed
status| |

--- Comment #23 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 346552:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=346552)

lgtm

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346553|none |reviewed
status| |

--- Comment #24 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 346553:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=346553)

lgtm

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #25 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Almost there. Though I would not push this code until we have a way to use it
in a pipeline.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Hyunjun Ko <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346551|needs-work |none
status| |
Attachment #346551|0 |1
is obsolete| |

--- Comment #26 from Hyunjun Ko <***@igalia.com> ---
Created attachment 348142
--> https://bugzilla.gnome.org/attachment.cgi?id=348142&action=edit
libs: encoder: add api gst_vaapi_encoder_add/del_roi

Implements and exposes new api gst_vaapi_encoder_add/del_roi to set ROI
regions.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #27 from Hyunjun Ko <***@igalia.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #25)

Post by "GStreamer" (GNOME Bugzilla)
Almost there. Though I would not push this code until we have a way to use
it in a pipeline.

I'm going to start working on GstEvent to support this.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #28 from Hyunjun Ko <***@igalia.com> ---
Recently, seems ROI is broken in the driver. I filed up this issue to
https://github.com/01org/intel-vaapi-driver/issues/108

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Hyunjun Ko <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348142|0 |1
is obsolete| |

--- Comment #29 from Hyunjun Ko <***@igalia.com> ---
Created attachment 348978
--> https://bugzilla.gnome.org/attachment.cgi?id=348978&action=edit
libs: encoder: add api gst_vaapi_encoder_add/del_roi

Implements and exposes new api gst_vaapi_encoder_add/del_roi to set ROI
regions.


-----
I revised this patch, especially about memory allocation happens inside api.
Please review this again.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #30 from Hyunjun Ko <***@igalia.com> ---
Created attachment 348979
--> https://bugzilla.gnome.org/attachment.cgi?id=348979&action=edit
vaapiencode: handle custom event GstVaapiEncoderRegionOfInterest

Handles new custom event GstVaapiEncoderRegionOfInterest
to enable/disable a ROI region.

Writes a way to use new event to document.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #31 from Hyunjun Ko <***@igalia.com> ---
Created attachment 348980
--> https://bugzilla.gnome.org/attachment.cgi?id=348980&action=edit
tests: elements: add an example for roi

This implements a pipleint to recognize difference between ROI and non-ROI.
See comments in this code in detail.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #32 from Hyunjun Ko <***@igalia.com> ---
Even though roi is broken on the current driver,
I would request review for a way to enable/disable roi.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #33 from Hyunjun Ko <***@igalia.com> ---
(In reply to Hyunjun Ko from comment #31)

Created attachment 348980 [details] [review]
tests: elements: add an example for roi
This implements a pipleint to recognize difference between ROI and non-ROI.
See comments in this code in detail.

I've been thinking about how to test and verify roi for a while.
This is an example, but if there's a better way, please let me know.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348978|none |reviewed
status| |

--- Comment #34 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 348978:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=348978)

this patch needs too be rebased because gstvaapiencoder.h has changed. But
still lgtm

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348979|none |needs-work
status| |

--- Comment #35 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 348979:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=348979)

::: gst/vaapi/gstvaapiencode.c
@@ +738,3 @@
(GstTaskFunction) gst_vaapiencode_buffer_loop, encode, NULL);
break;
+ case GST_EVENT_CUSTOM_DOWNSTREAM:

I wonder if this event should be GST_EVENT_CUSTOM_DOWNSTREAM_OOB (out of band)

@@ +741,3 @@
+ {
+ const GstStructure *s = gst_event_get_structure (event);
+ if (gst_structure_has_name (s, "GstVaapiEncoderRegionOfInterest")) {

I don't know if GstVaapiEncoderRegionOfInterest is a good name. Starting with
the namespace. I am not sure if other encoders could enable ROI.

We need to discuss it with the rest of the community. IMO it should be
something more like "region-of-interest"

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348980|none |reviewed
status| |

--- Comment #36 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 348980:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=348980)

This is beautiful! Great!

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #37 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
I don't know if this is a good timing to merge these patches, since we are in
the code freeze window, and the current version of Intel's driver (the only one
supporting this feature) has a regression on it (bug #780881).

Perhaps we might delay it until release 1.13/1.14 :S

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #38 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
I'm going to push these patches now.

Sree has some doubts about the usage of the GstEvent API for this. He thinks we
should find a way to emit it at a specific frame.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #39 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 346550:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=346550)

::: gst-libs/gst/vaapi/gstvaapiencoder_priv.h
@@ +231,3 @@
+ /* Region of Interest */
+ gboolean got_roi_capability;
+ gboolean roi_capability;

I feel these two variables are not required at all. The roi_capability is
repeated in the GstVaapiConfigInfoEncoder, and got_roi_capability doesn't
change much, since the query is fast, there's no need of cache it.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #40 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 346552:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=346552)

::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c
@@ +2270,3 @@
+ VAEncMiscParameterBufferROI *roi_param;
+ VAEncROI *region_roi, *region_ptr;
+ gint roi_num = g_list_length (base_encoder->roi_regions);

we are repeting twice the call g_list_length(), it would be better if we just
call it once, avoiding traversing the list for the same size.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #41 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 346553:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=346553)

::: tests/simple-encoder.c
@@ +34,3 @@
static char **g_input_files = NULL;
+static guint g_roi_enable = 0;
+static GstVaapiROI *g_roi_region[2] = { NULL, };

There's no need to keep this global, it could bie in the App structure.

@@ +191,3 @@
+
+ for (i = 0; i < 2; i++) {
+ g_roi_region[i] = g_malloc0 (sizeof (GstVaapiROI));

we can save this malloc&free using normal array declared in App structure.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #42 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 348978:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=348978)

::: gst-libs/gst/vaapi/gstvaapiencoder.c
@@ +1138,3 @@

+ if (encoder->roi_regions)
+ g_list_free (encoder->roi_regions);

here's a memory leak, since the allocated data are not freed.

@@ +1307,3 @@
+ region->rect.width == roi->rect.width &&
+ region->rect.height == roi->rect.height) {
+ encoder->roi_regions = g_list_remove (encoder->roi_regions, region);

here's a memory leak: we are not freeing the memory allocated for the
GstVaapiROO in _add_roi()

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #43 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 348980:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=348980)

::: tests/elements/test-roi.c
@@ +147,3 @@
+ " t. ! queue name=second_q ! " CODEC
+ " ! " VIDEO_ELEMENTS "!" TEXTOVERLAY "ROI "
+ " ! videobox border-alpha=0 left=-640 ! mix.", NULL);

this pipeline can be simplified a lot, avoiding many of those videoconverts,
and since it is only taking the snow part of videotestsrc, we can ask it to
generate only the snow pattern: pattern=snow
Thus we avoid the extract of that area.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #44 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 348979:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=348979)

::: gst/vaapi/gstvaapiencode.c
@@ +767,3 @@
+ }
+ }
+ break;

I would move this above the call to the parent, since this call only is handled
by us.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346550|reviewed |none
status| |
Attachment #346550|0 |1
is obsolete| |

--- Comment #45 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 351651
--> https://bugzilla.gnome.org/attachment.cgi?id=351651&action=edit
libs: encoder/context: query region of interest support

Queries if the driver supports "Region of Interest" (ROI) during the config
creation.

This attribute conveys whether the driver supports region-of-interest (ROI)
encoding, based on user provided ROI rectangles. The attribute value is
partitioned into fields as defined in the VAConfigAttribValEncROI union.

If ROI encoding is supported, the ROI information is passed to the driver
using VAEncMiscParameterTypeROI.

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Signed-off-by: Víctor Manuel Jáquez Leal <***@igalia.com>

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346552|reviewed |none
status| |
Attachment #346552|0 |1
is obsolete| |

--- Comment #47 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 351653
--> https://bugzilla.gnome.org/attachment.cgi?id=351653&action=edit
libs: encoder: h264: set ROI params during encoding

Set ROI params during encoding each frame, which are set via
gst_vaapi_encoder_add_roi ()

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Signed-off-by: Víctor Manuel Jáquez Leal <***@igalia.com>

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348978|reviewed |none
status| |
Attachment #348978|0 |1
is obsolete| |

--- Comment #46 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 351652
--> https://bugzilla.gnome.org/attachment.cgi?id=351652&action=edit
libs: encoder: add api gst_vaapi_encoder_add/del_roi

Implements and exposes new api gst_vaapi_encoder_add/del_roi to set ROI
regions.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #49 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 351655
--> https://bugzilla.gnome.org/attachment.cgi?id=351655&action=edit
vaapiencode: handle custom event GstVaapiEncoderRegionOfInterest

Handles new custom event GstVaapiEncoderRegionOfInterest
to enable/disable a ROI region.

Writes a way to use new event to document.

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Signed-off-by: Víctor Manuel Jáquez Leal <***@igalia.com>

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346553|reviewed |none
status| |
Attachment #346553|0 |1
is obsolete| |

--- Comment #48 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 351654
--> https://bugzilla.gnome.org/attachment.cgi?id=351654&action=edit
tests: simple-encoder: add an option to set ROI

$ simple-encoder -r inputfile.y4m

And you'll got an output file in H264 with two regions of interest.

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Signed-off-by: Víctor Manuel Jáquez Leal <***@igalia.com>

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #50 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 351656
--> https://bugzilla.gnome.org/attachment.cgi?id=351656&action=edit
tests: elements: add an example for ROI

This implements a pipleint to recognize difference between ROI and non-ROI.
See comments in this code in detail.

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Signed-off-by: Víctor Manuel Jáquez Leal <***@igalia.com>

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348980|0 |1
is obsolete| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348979|0 |1
is obsolete| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #51 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
@Hyunjun: I'll push these patches tomorrow. Please check the modifications I
did.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #52 from Hyunjun Ko <***@igalia.com> ---
Review of attachment 351652:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=351652)

::: gst-libs/gst/vaapi/gstvaapiencoder.c
@@ +1427,3 @@
+ /* Duplicated region */
+ goto end;
+ GstVaapiROI *region = NULL;

This break can be removed. Sorry for this stupid thing.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #53 from Hyunjun Ko <***@igalia.com> ---
Review of attachment 351655:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=351655)

::: gst/vaapi/gstvaapiencode.c
@@ +741,3 @@

+ switch (GST_EVENT_TYPE (event)) {
+ case GST_EVENT_CUSTOM_DOWNSTREAM:{

Maybe you want GST_EVENT_CUSTOM_DOWNSTREAM_OOB :)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #54 from Hyunjun Ko <***@igalia.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #51)

Post by "GStreamer" (GNOME Bugzilla)
@Hyunjun: I'll push these patches tomorrow. Please check the modifications I
did.

Thanks for supporting this work!
Please see my comments.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

--- Comment #55 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Attachment 351651 pushed as b41d72b - libs: encoder/context: query region of
interest support
Attachment 351652 pushed as 7a6f690 - libs: encoder: add api
gst_vaapi_encoder_add/del_roi
Attachment 351653 pushed as f3302a0 - libs: encoder: h264: set ROI params
during encoding
Attachment 351654 pushed as c21345c - tests: simple-encoder: add an option to
set ROI
Attachment 351655 pushed as 8f1b88d - vaapiencode: handle custom event
GstVaapiEncoderRegionOfInterest
Attachment 351656 pushed as eb17b71 - tests: elements: add an example for ROI

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #351651|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #351652|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #351653|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #351654|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #351656|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #351655|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|git master |1.13.1

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |REOPENED
CC| |***@ndufresne.ca
Depends on| |793338
Resolution|FIXED |---
Severity|enhancement |blocker

--- Comment #56 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
I'm reopening this one since I think the custom event has design issues. It
does not support ROI hierarchy, does not have ROI ID (making removal
ambiguous), removing ROI requires keeping state and setting again the ROI which
is highly error prone.

I believe this should use GstVideoRegionOfInterestMeta with the proposal at
https://bugzilla.gnome.org/show_bug.cgi?id=793338 . Just a reminder that as a
mainline repository, it's important to think about feature to be generally
useful. This custom event was done a bit "downstream" style.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Guillaume Desmottes <***@gnome.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gnome.org

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #57 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
I'm okay reopening this issue. But is it a blocker?

Post by "GStreamer" (GNOME Bugzilla)
Just a reminder that as
a mainline repository, it's important to think about feature to be generally
useful. This custom event was done a bit "downstream" style.

Agree, but our intention was to have a proof of concept without modifying the
core libraries of gstreamer. The problem would be if there are current users of
the downstream event. I'm not aware of any. And of course I celebrate the
effort to bring this feature to upstream.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #58 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
I don't want the custom event to become part of the supported API.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #59 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Ok. My work plan is to revert attachment 351655 and later rework the code to
match with the meta

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248
Bug 768248 depends on bug 793338, which changed state.

Bug 793338 Summary: videometa: add support for encoder parameters to ROI meta
https://bugzilla.gnome.org/show_bug.cgi?id=793338

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #60 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Or just forward port, I was about to look at, shouldn't be very hard really.
It's event less work since you no longer need to keep a state.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #61 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Do you have a test app for the roimeta? An app that injects roimeta through
identity or something?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #62 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
(In reply to Víctor Manuel Jáquez Leal from comment #61)

Post by "GStreamer" (GNOME Bugzilla)
Do you have a test app for the roimeta? An app that injects roimeta through
identity or something?

We have one for roi/om-zynq, which I'll try to get ready for 1.14. We use
facedetect ! omxh264enc with a pad probe in between. We have other elements (HW
accelerated) that started injecting ROI. I think textoverlay should add an ROI
too.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #63 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 368838
--> https://bugzilla.gnome.org/attachment.cgi?id=368838&action=edit
Revert "vaapiencode: handle custom event GstVaapiEncoderRegionOfInterest"

This reverts commit 8f1b88dac0e64a211325cdcb2cda693b80229bd1.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #64 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 368839
--> https://bugzilla.gnome.org/attachment.cgi?id=368839&action=edit
Revert "tests: simple-encoder: add an option to set ROI"

This reverts commit c21345c4787bb6342adddea1190f53fe62abff04.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #66 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 368841
--> https://bugzilla.gnome.org/attachment.cgi?id=368841&action=edit
libs: encoder: reimplement ROI using meta

Check input buffers for ROI metas and pass them to VA. Also added a
new "default-roi-delta-qp" property in order to tell the encoder what
delta QP should be applied to ROI by default.

Enabled it for H264 and H265 encoders.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #65 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 368840
--> https://bugzilla.gnome.org/attachment.cgi?id=368840&action=edit
Revert "libs: encoder: add api gst_vaapi_encoder_add/del_roi"

This reverts commit 7a6f690340dcb3b82c59efa777d4453227851de8.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #67 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 368842
--> https://bugzilla.gnome.org/attachment.cgi?id=368842&action=edit
plugins: copy input buffer metas

When importing buffers to a VA-base buffer, it is required to copy
the metas in the original buffer, otherwise information will be
lost, such as GstVideoRegionOfInterestMeta.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #68 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 368843
--> https://bugzilla.gnome.org/attachment.cgi?id=368843&action=edit
test: wip: roi test

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #368843|0 |1
is obsolete| |

--- Comment #69 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 368845
--> https://bugzilla.gnome.org/attachment.cgi?id=368845&action=edit
test: wip: roi test

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #70 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
with intel-vaapi-driver in kabylake (my laptop) H264 doesn't support ROI
anymore, but H265 does. I enabled H265, but my test is very flaky.

but at least it handles the same API.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #71 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Thanks Victor, I'll review today. Do you have some reference on which HW will
support ROI ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #72 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
There's a tool written by Mark Thomson that returns the capabilities of the
used driver.

I modified a bit for the libva's ROI typo :)

https://people.igalia.com/vjaquez/vadumpcaps.c

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #368841|none |accepted-commit_now
status| |

--- Comment #73 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Review of attachment 368841:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=368841)

Looks good.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #368842|none |accepted-commit_now
status| |

--- Comment #74 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Review of attachment 368842:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=368842)

So familiar ;-P

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #75 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Review of attachment 368845:
--> (https://bugzilla.gnome.org/review?bug=768248&attachment=368845)

What's needed to finish this ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #76 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
It looks all good to me, just need to finish the test I guess.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #77 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
I think that the ROI seems to get lost along the way in:

gst-launch-1.0 -e v4l2src ! videoconvert ! facedetect
profile=haarcascade_frontalface_default.xml ! videoconvert ! queue !
vaapipostproc ! vaapih264enc default-roi-delta-qp=10 rate-control=cbr
bitrate=512 ! ...

like if "plugins: copy input buffer metas" had no effect.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #78 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
(In reply to Nicolas Dufresne (stormer) from comment #77)

Post by "GStreamer" (GNOME Bugzilla)
gst-launch-1.0 -e v4l2src ! videoconvert ! facedetect
profile=haarcascade_frontalface_default.xml ! videoconvert ! queue !
vaapipostproc ! vaapih264enc default-roi-delta-qp=10 rate-control=cbr
bitrate=512 ! ...
like if "plugins: copy input buffer metas" had no effect.

Did you check with vadumpcaps if your driver/backend supports roi with h264??

In my kabylake it doesn't, but it does for h265

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #368842|accepted-commit_now |committed
status| |

--- Comment #79 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Comment on attachment 368842
--> https://bugzilla.gnome.org/attachment.cgi?id=368842
plugins: copy input buffer metas

Attachment 368842 pushed as ba28c6cf - plugins: copy input buffer metas

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #368838|0 |1
is obsolete| |

--- Comment #80 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 369063
--> https://bugzilla.gnome.org/attachment.cgi?id=369063&action=edit
Revert "vaapiencode: handle custom event GstVaapiEncoderRegionOfInterest"

This reverts commit 8f1b88dac0e64a211325cdcb2cda693b80229bd1.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #368840|0 |1
is obsolete| |

--- Comment #82 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 369065
--> https://bugzilla.gnome.org/attachment.cgi?id=369065&action=edit
Revert "libs: encoder: add api gst_vaapi_encoder_add/del_roi"

This reverts commit 7a6f690340dcb3b82c59efa777d4453227851de8.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #368839|0 |1
is obsolete| |

--- Comment #81 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 369064
--> https://bugzilla.gnome.org/attachment.cgi?id=369064&action=edit
Revert "tests: simple-encoder: add an option to set ROI"

This reverts commit c21345c4787bb6342adddea1190f53fe62abff04.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

--- Comment #84 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 369067
--> https://bugzilla.gnome.org/attachment.cgi?id=369067&action=edit
tests: element: rewrite ROI test

Rewrote the ROI test to use GstVideoRegionOfInterest meta rather
than injecting GstEvents. These meta are added as a pad probe in
the queue src pad.

Also

* Use of navigation messages to control de test
* Use signal watch for processing messages
* Change to H265 rather than H264 since current intel-vaapi-driver
only supports ROI on kabylake.
TODO: add a parameter to change the encoder/decoder to test.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #368841|accepted-commit_now |none
status| |
Attachment #368841|0 |1
is obsolete| |

--- Comment #83 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 369066
--> https://bugzilla.gnome.org/attachment.cgi?id=369066&action=edit
libs: encoder: reimplement ROI using meta

Check input buffers for ROI metas and pass them to VA. Also added a
new "default-roi-delta-qp" property in order to tell the encoder what
delta QP should be applied to ROI by default.

Enabled it for H264 and H265 encoders.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=768248

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #368845|0 |1
is obsolete| |