GNOME Bugzilla – Bug 768248
vaapiencode: Add Encoding region-of-interest (ROI) support
Last modified: 2018-03-01 23:20:35 UTC
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.
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 :)
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.
Created attachment 344625 [details] [review] libs: encoder/context: query region of interest support Query Region of Interest support during config creation.
Created attachment 344626 [details] [review] libs: encoder: add api gst_vaapi_encoder_set_roi Implements and exposes new api gst_vaapi_encoder_set_roi to set ROI regions.
Created attachment 344627 [details] [review] libs: encoder: h264: set ROI params during encoding Set ROI params during encoding each frame, which is set via gst_vaapi_encoder_set_roi
Created attachment 344628 [details] [review] tests: simple-encoder: add an option to set ROI Adds an option "roi" to test roi support of encoder.
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.
Ping. Victor, Sree? :)
Review of attachment 344625 [details] [review]: ::: 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.
Review of attachment 344626 [details] [review]: ::: 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);
Review of attachment 344627 [details] [review]: lgtm
Review of attachment 344628 [details] [review]: 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.
Review of attachment 344626 [details] [review]: ::: 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
(In reply to Víctor Manuel Jáquez Leal from comment #9) > Review of attachment 344625 [details] [review] [review]: > > ::: 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. > ::: 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. > > 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.
(In reply to Víctor Manuel Jáquez Leal from comment #10) > Review of attachment 344626 [details] [review] [review]: > > ::: 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.
(In reply to Víctor Manuel Jáquez Leal from comment #13) > Review of attachment 344626 [details] [review] [review]: > > ::: 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.
Created attachment 346550 [details] [review] libs: encoder/context: query region of interest support Query Region of Interest support during config creation.
Created attachment 346551 [details] [review] 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.
Created attachment 346552 [details] [review] libs: encoder: h264: set ROI params during encoding Set ROI params during encoding each frame, which is set via gst_vaapi_encoder_add_roi
Created attachment 346553 [details] [review] tests: simple-encoder: add an option to set ROI
Review of attachment 346551 [details] [review]: ::: 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.
Review of attachment 346550 [details] [review]: lgtm
Review of attachment 346552 [details] [review]: lgtm
Review of attachment 346553 [details] [review]: lgtm
Almost there. Though I would not push this code until we have a way to use it in a pipeline.
Created attachment 348142 [details] [review] 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.
(In reply to Víctor Manuel Jáquez Leal from comment #25) > 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.
Recently, seems ROI is broken in the driver. I filed up this issue to https://github.com/01org/intel-vaapi-driver/issues/108
Created attachment 348978 [details] [review] 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.
Created attachment 348979 [details] [review] 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.
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.
Even though roi is broken on the current driver, I would request review for a way to enable/disable roi.
(In reply to Hyunjun Ko from comment #31) > Created attachment 348980 [details] [review] [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.
Review of attachment 348978 [details] [review]: this patch needs too be rebased because gstvaapiencoder.h has changed. But still lgtm
Review of attachment 348979 [details] [review]: ::: 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"
Review of attachment 348980 [details] [review]: This is beautiful! Great!
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
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.
Review of attachment 346550 [details] [review]: ::: 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.
Review of attachment 346552 [details] [review]: ::: 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.
Review of attachment 346553 [details] [review]: ::: 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.
Review of attachment 348978 [details] [review]: ::: 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()
Review of attachment 348980 [details] [review]: ::: 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.
Review of attachment 348979 [details] [review]: ::: 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.
Created attachment 351651 [details] [review] 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 <vjaquez@igalia.com>
Created attachment 351652 [details] [review] 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.
Created attachment 351653 [details] [review] 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 <vjaquez@igalia.com>
Created attachment 351654 [details] [review] 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 <vjaquez@igalia.com>
Created attachment 351655 [details] [review] 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 <vjaquez@igalia.com>
Created attachment 351656 [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. https://bugzilla.gnome.org/show_bug.cgi?id=768248 Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
@Hyunjun: I'll push these patches tomorrow. Please check the modifications I did.
Review of attachment 351652 [details] [review]: ::: 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.
Review of attachment 351655 [details] [review]: ::: gst/vaapi/gstvaapiencode.c @@ +741,3 @@ + switch (GST_EVENT_TYPE (event)) { + case GST_EVENT_CUSTOM_DOWNSTREAM:{ Maybe you want GST_EVENT_CUSTOM_DOWNSTREAM_OOB :)
(In reply to Víctor Manuel Jáquez Leal from comment #51) > @Hyunjun: I'll push these patches tomorrow. Please check the modifications I > did. Thanks for supporting this work! Please see my comments.
Attachment 351651 [details] pushed as b41d72b - libs: encoder/context: query region of interest support Attachment 351652 [details] pushed as 7a6f690 - libs: encoder: add api gst_vaapi_encoder_add/del_roi Attachment 351653 [details] pushed as f3302a0 - libs: encoder: h264: set ROI params during encoding Attachment 351654 [details] pushed as c21345c - tests: simple-encoder: add an option to set ROI Attachment 351655 [details] pushed as 8f1b88d - vaapiencode: handle custom event GstVaapiEncoderRegionOfInterest Attachment 351656 [details] pushed as eb17b71 - tests: elements: add an example for ROI
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.
I'm okay reopening this issue. But is it a blocker? > 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.
I don't want the custom event to become part of the supported API.
Ok. My work plan is to revert attachment 351655 [details] [review] and later rework the code to match with the meta
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.
Do you have a test app for the roimeta? An app that injects roimeta through identity or something?
(In reply to Víctor Manuel Jáquez Leal from comment #61) > 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.
Created attachment 368838 [details] [review] Revert "vaapiencode: handle custom event GstVaapiEncoderRegionOfInterest" This reverts commit 8f1b88dac0e64a211325cdcb2cda693b80229bd1.
Created attachment 368839 [details] [review] Revert "tests: simple-encoder: add an option to set ROI" This reverts commit c21345c4787bb6342adddea1190f53fe62abff04.
Created attachment 368840 [details] [review] Revert "libs: encoder: add api gst_vaapi_encoder_add/del_roi" This reverts commit 7a6f690340dcb3b82c59efa777d4453227851de8.
Created attachment 368841 [details] [review] 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.
Created attachment 368842 [details] [review] 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.
Created attachment 368843 [details] [review] test: wip: roi test
Created attachment 368845 [details] [review] test: wip: roi test
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.
Thanks Victor, I'll review today. Do you have some reference on which HW will support ROI ?
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
Review of attachment 368841 [details] [review]: Looks good.
Review of attachment 368842 [details] [review]: So familiar ;-P
Review of attachment 368845 [details] [review]: What's needed to finish this ?
It looks all good to me, just need to finish the test I guess.
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.
(In reply to Nicolas Dufresne (stormer) from comment #77) > 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. Did you check with vadumpcaps if your driver/backend supports roi with h264?? In my kabylake it doesn't, but it does for h265
Comment on attachment 368842 [details] [review] plugins: copy input buffer metas Attachment 368842 [details] pushed as ba28c6cf - plugins: copy input buffer metas
Created attachment 369063 [details] [review] Revert "vaapiencode: handle custom event GstVaapiEncoderRegionOfInterest" This reverts commit 8f1b88dac0e64a211325cdcb2cda693b80229bd1.
Created attachment 369064 [details] [review] Revert "tests: simple-encoder: add an option to set ROI" This reverts commit c21345c4787bb6342adddea1190f53fe62abff04.
Created attachment 369065 [details] [review] Revert "libs: encoder: add api gst_vaapi_encoder_add/del_roi" This reverts commit 7a6f690340dcb3b82c59efa777d4453227851de8.
Created attachment 369066 [details] [review] 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.
Created attachment 369067 [details] [review] 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.
(In reply to Víctor Manuel Jáquez Leal from comment #78) > (In reply to Nicolas Dufresne (stormer) from comment #77) > > 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. > > Did you check with vadumpcaps if your driver/backend supports roi with h264?? > > In my kabylake it doesn't, but it does for h265 No, but I can confirm that vaapipostproc does not copy the meta, fixing it. I've just validating the ROI makes it to the encoder. The code seems to think that this is supported.
Created attachment 369068 [details] [review] postproc: Copy meta data from input to output This will ensure that meta data without memory tags will be copied. This was noticed when testing ROI.
I believe this patch is correct, it does copy the ROI, but now it crashes badly.
I see this in valgrind (and more): ==421== Invalid write of size 2 ==421== at 0x360DEAD7: gst_vaapi_encoder_ensure_param_roi_regions (gstvaapiencoder.c:341) ==421== by 0x360E8DC2: ensure_misc_params (gstvaapiencoder_h264.c:2540) ==421== by 0x360E8DC2: gst_vaapi_encoder_h264_encode (gstvaapiencoder_h264.c:2867) ==421== by 0x360DED40: gst_vaapi_encoder_put_frame (gstvaapiencoder.c:512) ==421== by 0x360B3EB9: gst_vaapiencode_handle_frame (gstvaapiencode.c:691) ==421== by 0xF856F8E: gst_video_encoder_chain (gstvideoencoder.c:1548) ==421== by 0x4EBA69A: gst_pad_chain_data_unchecked (gstpad.c:4275) ==421== by 0x4EBA69A: gst_pad_push_data (gstpad.c:4531) ==421== by 0x4EC2802: gst_pad_push (gstpad.c:4650) ==421== by 0xF5EDA1F: gst_base_transform_chain (gstbasetransform.c:2321) ==421== by 0x4EBA69A: gst_pad_chain_data_unchecked (gstpad.c:4275) ==421== by 0x4EBA69A: gst_pad_push_data (gstpad.c:4531) ==421== by 0x4EC2802: gst_pad_push (gstpad.c:4650) ==421== by 0xF5EDA1F: gst_base_transform_chain (gstbasetransform.c:2321) ==421== by 0x4EBA69A: gst_pad_chain_data_unchecked (gstpad.c:4275) ==421== by 0x4EBA69A: gst_pad_push_data (gstpad.c:4531) ==421== Address 0xedc2768 is 168 bytes inside an unallocated block of size 1,072 in arena "client" Which leads to this (probably just a side effect): ==421== Invalid read of size 2 ==421== at 0x389A3ECA: intel_encoder_check_roi_parameter (i965_encoder.c:671) ==421== by 0x389A3ECA: intel_encoder_check_brc_parameter (i965_encoder.c:726) ==421== by 0x389A3ECA: intel_encoder_check_misc_parameter (i965_encoder.c:861) ==421== by 0x389A3ECA: intel_encoder_sanity_check_input (i965_encoder.c:1335) ==421== by 0x389A3ECA: intel_encoder_end_picture (i965_encoder.c:1351) ==421== by 0x3634B28F: vaEndPicture (va.c:1316) ==421== by 0x360EE4AF: gst_vaapi_enc_picture_encode (gstvaapiencoder_objects.c:590) ==421== by 0x360E926C: gst_vaapi_encoder_h264_encode (gstvaapiencoder_h264.c:2873) ==421== by 0x360DED40: gst_vaapi_encoder_put_frame (gstvaapiencoder.c:512) ==421== by 0x360B3EB9: gst_vaapiencode_handle_frame (gstvaapiencode.c:691) ==421== by 0xF856F8E: gst_video_encoder_chain (gstvideoencoder.c:1548) ==421== by 0x4EBA69A: gst_pad_chain_data_unchecked (gstpad.c:4275) ==421== by 0x4EBA69A: gst_pad_push_data (gstpad.c:4531) ==421== by 0x4EC2802: gst_pad_push (gstpad.c:4650) ==421== by 0xF5EDA1F: gst_base_transform_chain (gstbasetransform.c:2321) ==421== by 0x4EBA69A: gst_pad_chain_data_unchecked (gstpad.c:4275) ==421== by 0x4EBA69A: gst_pad_push_data (gstpad.c:4531) ==421== by 0x4EC2802: gst_pad_push (gstpad.c:4650) ==421== Address 0xedc2768 is 24 bytes after a block of size 32 in arena "client"
Review of attachment 369066 [details] [review]: Found what cause the crashes. ::: gst-libs/gst/vaapi/gstvaapiencoder.c @@ +311,3 @@ + + region_roi = (VAEncROI *) misc->param + sizeof (VAEncMiscParameterBuffer) + + sizeof (VAEncMiscParameterBufferROI); Should be: region_roi = (VAEncROI *) ((guint8*) misc->param + sizeof (VAEncMiscParameterBuffer) + sizeof (VAEncMiscParameterBufferROI)); Otherwise you are adding VAEncROI instead of bytes. There use to be a guchar* cast in the old code.
<3!!! :)
Created attachment 369071 [details] [review] 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.
Created attachment 369072 [details] [review] 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.
Thanks for porting and fixing the bugs with copying the params. This is greatly appriciated. So I merged this port now, I think we can forward patch if we find any issues. So far it does not have any bad effect, which is the goal. My Ivibridge here pretends to do ROI on H264, but I could got get any visually visible effect so this might be a lie. But I could calidate that the information seems to travel to the encoder. I'll try again on kabylake later. Attachment 369063 [details] pushed as 25c2a0d - Revert "vaapiencode: handle custom event GstVaapiEncoderRegionOfInterest" Attachment 369064 [details] pushed as 35226db - Revert "tests: simple-encoder: add an option to set ROI" Attachment 369065 [details] pushed as d311071 - Revert "libs: encoder: add api gst_vaapi_encoder_add/del_roi" Attachment 369068 [details] pushed as fde5500 - postproc: Copy meta data from input to output Attachment 369071 [details] pushed as c49a17e - libs: encoder: reimplement ROI using meta Attachment 369072 [details] pushed as e82fafc - tests: element: rewrite ROI test
Review of attachment 369068 [details] [review]: Thanks! But it is incomplete, deinterlaced buffers are not considered. I can update the patch later :)
I'm not sure how, but I guess I'll understand when I see your patch. It is relatively minor, I mainly wanted to restore the feature before today's freeze. That also makes me wonder if meta traverse the software deinterlacer properly.
I have pushed a commit refactoring the way how the metadata is copied to vpp output buffers.