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

[GStreamer (GNOME Bugzilla)]

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

--- Comment #14 from Hyunjun Ko <zzoon at igalia.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #9)
> 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.

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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.