Make vaapidecode to advertise the cropped values in srcpad, but negotiate pool only if needed

-- Maintaing decoded surface resoluton and actual display resoultion separately
-- Before pushing every frames downstream, check for the requirement of pool negoation and
output_state negotiation: This is needed to avoid multiple issuses with cropping,
multi-resoluton video handling, more complex multi resolution decode scenarios for vp9decode,
possible wrong behaviour from upstream element to report uncropped values etc. Due to these reasons,
We can't just reliably use the resolution change notification from libgstvaapi for pool renegotiation too.
This is slight overhead, but safe enough. Optimization could be possible though.

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

gst/vaapi/gstvaapidecode.c

@@ -140,6 +140,7 @@ static gboolean gst_vaapi_decode_input_state_replace (GstVaapiDecode * decode,
     const GstVideoCodecState * new_state);
 static gboolean gst_vaapidecode_negotiate (GstVaapiDecode * decode);
 
+/* get invoked only if actural VASurface size (not the cropped values) changed */
 static void
 gst_vaapi_decoder_state_changed (GstVaapiDecoder * decoder,
     const GstVideoCodecState * codec_state, gpointer user_data)
@@ -153,7 +154,7 @@ gst_vaapi_decoder_state_changed (GstVaapiDecoder * decoder,
   if (!gst_vaapidecode_update_sink_caps (decode, decode->input_state->caps))
     return;
 
-  decode->do_renego = TRUE;
+  decode->do_pool_renego = TRUE;
 }
 
 static GstVideoCodecState *
@@ -218,6 +219,7 @@ gst_vaapidecode_update_src_caps (GstVaapiDecode * decode)
   GstVideoFormat format = GST_VIDEO_FORMAT_I420;
   GstClockTime latency;
   gint fps_d, fps_n;
+  guint width, height;
 
   if (!decode->input_state)
     return FALSE;
@@ -247,8 +249,16 @@ gst_vaapidecode_update_src_caps (GstVaapiDecode * decode)
       break;
   }
 
+  width = GST_VIDEO_INFO_WIDTH (&decode->display_info);
+  height = GST_VIDEO_INFO_HEIGHT (&decode->display_info);
+
+  if (!width || !height) {
+    width = ref_state->info.width;
+    height = ref_state->info.height;
+  }
+
   state = gst_video_decoder_set_output_state (vdec, format,
-      ref_state->info.width, ref_state->info.height, ref_state);
+      width, height, ref_state);
   if (!state || state->info.width == 0 || state->info.height == 0) {
     if (features)
       gst_caps_features_free (features);
@@ -281,6 +291,7 @@ gst_vaapidecode_update_src_caps (GstVaapiDecode * decode)
   latency = gst_util_uint64_scale (2 * GST_SECOND, fps_d, fps_n);
   gst_video_decoder_set_latency (vdec, latency, latency);
 
+  decode->do_outstate_renego = FALSE;
   return TRUE;
 }
 
@@ -293,26 +304,83 @@ gst_vaapidecode_release (GstVaapiDecode * decode)
   gst_object_unref (decode);
 }
 
+/* check whether the decoded surface size has changed */
 static gboolean
-is_surface_resolution_changed (GstVideoDecoder * vdec,
+is_surface_resolution_changed (GstVaapiDecode * decode,
     GstVaapiSurface * surface)
 {
+  GstVideoDecoder *const vdec = GST_VIDEO_DECODER (decode);
+  GstVideoInfo *vinfo = &decode->decoded_info;
+  GstVideoFormat format = GST_VIDEO_FORMAT_ENCODED;
   guint surface_width, surface_height;
-  guint configured_width, configured_height;
   GstVideoCodecState *state;
-  gboolean ret = FALSE;
+
+  g_return_val_if_fail (surface != NULL, FALSE);
 
   gst_vaapi_surface_get_size (surface, &surface_width, &surface_height);
 
+  if (GST_VIDEO_INFO_WIDTH (vinfo) == surface_width
+      && GST_VIDEO_INFO_HEIGHT (vinfo) == surface_height)
+    return FALSE;
+
   state = gst_video_decoder_get_output_state (vdec);
-  configured_width = GST_VIDEO_INFO_WIDTH (&state->info);
+  if (state) {
-  configured_height = GST_VIDEO_INFO_HEIGHT (&state->info);
+    /* Fixme: Get exact surface format usings gst_vaapi_surface_get_format () */
-  gst_video_codec_state_unref (state);
+    format = GST_VIDEO_INFO_FORMAT (&state->info);
+    gst_video_codec_state_unref (state);
+  }
+  gst_video_info_set_format (vinfo, format, surface_width, surface_height);
 
-  if (surface_width != configured_width || surface_height != configured_height)
+  return TRUE;
-    ret = TRUE;
+}
 
-  return ret;
+/* check whether display resolution changed */
+static gboolean
+is_display_resolution_changed (GstVaapiDecode * decode,
+    const GstVaapiRectangle * crop_rect)
+{
+  GstVideoDecoder *const vdec = GST_VIDEO_DECODER (decode);
+  GstVideoFormat format = GST_VIDEO_FORMAT_ENCODED;
+  GstVideoCodecState *state;
+  GstVideoInfo *vinfo;
+  guint display_width = GST_VIDEO_INFO_WIDTH (&decode->decoded_info);
+  guint display_height = GST_VIDEO_INFO_HEIGHT (&decode->decoded_info);
+
+  if (crop_rect) {
+    display_width = crop_rect->width;
+    display_height = crop_rect->height;
+  }
+  state = gst_video_decoder_get_output_state (vdec);
+  if (G_UNLIKELY (!state))
+    goto set_display_res;
+  vinfo = &state->info;
+  format = GST_VIDEO_INFO_FORMAT (vinfo);
+
+  if (GST_VIDEO_INFO_FORMAT (&decode->display_info) == GST_VIDEO_FORMAT_UNKNOWN)
+    decode->display_info = *vinfo;
+
+  if (!crop_rect) {
+    display_width = GST_VIDEO_INFO_WIDTH (&decode->decoded_info);
+    display_height = GST_VIDEO_INFO_HEIGHT (&decode->decoded_info);
+
+    if (G_UNLIKELY (display_width !=
+            GST_VIDEO_INFO_WIDTH (&decode->display_info)
+            || display_height != GST_VIDEO_INFO_HEIGHT (&decode->display_info)))
+      goto set_display_res;
+  }
+
+  if (GST_VIDEO_INFO_WIDTH (vinfo) == display_width
+      && GST_VIDEO_INFO_HEIGHT (vinfo) == display_height) {
+    gst_video_codec_state_unref (state);
+    return FALSE;
+  }
+
+set_display_res:
+  gst_video_info_set_format (&decode->display_info, format, display_width,
+      display_height);
+  if (state)
+    gst_video_codec_state_unref (state);
+  return TRUE;
 }
 
 static GstFlowReturn
@@ -328,17 +396,29 @@ gst_vaapidecode_push_decoded_frame (GstVideoDecoder * vdec,
 
   if (!GST_VIDEO_CODEC_FRAME_IS_DECODE_ONLY (out_frame)) {
     proxy = gst_video_codec_frame_get_user_data (out_frame);
+    crop_rect = gst_vaapi_surface_proxy_get_crop_rect (proxy);
+
+    /* in theory, we are not supposed to check the surface resolution
+     * change here since it should be advertised before from ligstvaapi.
+     * But there are issues with it especially for some vp9 streams where
+     * upstream element set un-cropped values in set_format() which make
+     * everything a mess. So better doing the explicit check here irrespective
+     * of what notification we get from upstream or libgstvaapi */
+    decode->do_pool_renego =
+        is_surface_resolution_changed (decode,
+        GST_VAAPI_SURFACE_PROXY_SURFACE (proxy));
+
+    decode->do_outstate_renego =
+        is_display_resolution_changed (decode, crop_rect);
 
     if (G_UNLIKELY (!decode->active) ||
         gst_pad_needs_reconfigure (GST_VIDEO_DECODER_SRC_PAD (vdec)) ||
-        decode->do_renego ||
+        decode->do_outstate_renego || decode->do_pool_renego) {
-        is_surface_resolution_changed (vdec,
+
-            GST_VAAPI_SURFACE_PROXY_SURFACE (proxy))) {
       if (!gst_vaapidecode_negotiate (decode))
         return GST_FLOW_ERROR;
 
       decode->active = TRUE;
-      decode->do_renego = FALSE;
     }
 
     gst_vaapi_surface_proxy_set_destroy_notify (proxy,
@@ -372,7 +452,6 @@ gst_vaapidecode_push_decoded_frame (GstVideoDecoder * vdec,
           GST_VIDEO_BUFFER_FLAG_FIRST_IN_BUNDLE);
     }
 
-    crop_rect = gst_vaapi_surface_proxy_get_crop_rect (proxy);
     if (crop_rect) {
       GstVideoCropMeta *const crop_meta =
           gst_buffer_add_video_crop_meta (out_frame->output_buffer);
@@ -616,8 +695,22 @@ gst_vaapidecode_decide_allocation (GstVideoDecoder * vdec, GstQuery * query)
       GST_VAAPI_CAPS_FEATURE_GL_TEXTURE_UPLOAD_META);
 #endif
 
-  return gst_vaapi_plugin_base_decide_allocation (GST_VAAPI_PLUGIN_BASE (vdec),
+  if (decode->do_pool_renego) {
-      query, 0);
+    gboolean ret;
+
+    caps = gst_caps_copy (caps);
+    gst_caps_set_simple (caps, "width", G_TYPE_INT,
+        GST_VIDEO_INFO_WIDTH (&decode->decoded_info), "height", G_TYPE_INT,
+        GST_VIDEO_INFO_HEIGHT (&decode->decoded_info), NULL);
+    ret =
+        gst_vaapi_plugin_base_decide_allocation (GST_VAAPI_PLUGIN_BASE (vdec),
+        query, 0, caps);
+    gst_caps_unref (caps);
+    decode->do_pool_renego = FALSE;
+    return ret;
+  } else {
+    return TRUE;
+  }
 }
 
 static inline gboolean
@@ -823,8 +916,6 @@ gst_vaapidecode_open (GstVideoDecoder * vdec)
   if (old_display)
     gst_vaapi_display_unref (old_display);
 
-  decode->do_renego = TRUE;
-
   return success;
 }
 
@@ -1146,10 +1237,15 @@ gst_vaapidecode_init (GstVaapiDecode * decode)
   decode->decoder = NULL;
   decode->decoder_caps = NULL;
   decode->allowed_caps = NULL;
+  decode->do_outstate_renego = TRUE;
+  decode->do_pool_renego = TRUE;
 
   g_mutex_init (&decode->surface_ready_mutex);
   g_cond_init (&decode->surface_ready);
 
+  gst_video_info_init (&decode->decoded_info);
+  gst_video_info_init (&decode->display_info);
+
   gst_video_decoder_set_packetized (vdec, FALSE);
 }
 

gst/vaapi/gstvaapidecode.h

@@ -41,6 +41,8 @@ struct _GstVaapiDecode {
 
     GstCaps            *sinkpad_caps;
     GstCaps            *srcpad_caps;
+    GstVideoInfo        decoded_info;
+    GstVideoInfo        display_info;
     GstVaapiDecoder    *decoder;
     GMutex              surface_ready_mutex;
     GCond               surface_ready;
@@ -51,7 +53,8 @@ struct _GstVaapiDecode {
 
     GstVideoCodecState *input_state;
     volatile gboolean   active;
-    volatile gboolean   do_renego;
+    volatile gboolean   do_outstate_renego;
+    volatile gboolean   do_pool_renego;
 };
 
 struct _GstVaapiDecodeClass {

gst/vaapi/gstvaapipluginbase.c

@@ -587,6 +587,8 @@ gst_vaapi_plugin_base_set_pool_config (GstBufferPool * pool,
  * @query: the allocation query to parse
  * @feature: the desired #GstVaapiCapsFeature, or zero to find the
  *   preferred one
+ * @preferred_caps: the desired #GstCaps, or NULL to find the
+ *   preferred one from query
  *
  * Decides allocation parameters for the downstream elements.
  *
@@ -594,11 +596,12 @@ gst_vaapi_plugin_base_set_pool_config (GstBufferPool * pool,
  */
 gboolean
 gst_vaapi_plugin_base_decide_allocation (GstVaapiPluginBase * plugin,
-    GstQuery * query, guint feature)
+    GstQuery * query, guint feature, GstCaps * preferred_caps)
 {
   GstCaps *caps = NULL;
   GstBufferPool *pool;
   GstStructure *config;
+  GstQuery *new_query;
   GstVideoInfo vi;
   guint size, min, max;
   gboolean update_pool = FALSE;
@@ -613,6 +616,15 @@ gst_vaapi_plugin_base_decide_allocation (GstVaapiPluginBase * plugin,
 
   gst_query_parse_allocation (query, &caps, NULL);
 
+  /* Make sure new caps get advertised to all downstream elements */
+  if (preferred_caps) {
+    new_query = gst_query_new_allocation (preferred_caps, FALSE);
+    if (!gst_pad_peer_query (GST_VAAPI_PLUGIN_BASE_SRC_PAD (plugin), new_query)) {
+      GST_DEBUG ("didn't get downstream ALLOCATION hints");
+    }
+    gst_query_unref (new_query);
+  }
+
   /* We don't need any GL context beyond this point if not requested
      so explicitly through GstVideoGLTextureUploadMeta */
   gst_object_replace (&plugin->gl_context, NULL);
@@ -657,7 +669,11 @@ gst_vaapi_plugin_base_decide_allocation (GstVaapiPluginBase * plugin,
     goto error_ensure_display;
 
   gst_video_info_init (&vi);
-  gst_video_info_from_caps (&vi, caps);
+  if (!preferred_caps)
+    gst_video_info_from_caps (&vi, caps);
+  else
+    gst_video_info_from_caps (&vi, preferred_caps);
+
   if (GST_VIDEO_INFO_FORMAT (&vi) == GST_VIDEO_FORMAT_ENCODED)
     gst_video_info_set_format (&vi, GST_VIDEO_FORMAT_I420,
         GST_VIDEO_INFO_WIDTH (&vi), GST_VIDEO_INFO_HEIGHT (&vi));
@@ -690,7 +706,11 @@ gst_vaapi_plugin_base_decide_allocation (GstVaapiPluginBase * plugin,
       goto error_create_pool;
 
     config = gst_buffer_pool_get_config (pool);
-    gst_buffer_pool_config_set_params (config, caps, size, min, max);
+    if (!preferred_caps)
+      gst_buffer_pool_config_set_params (config, caps, size, min, max);
+    else
+      gst_buffer_pool_config_set_params (config, preferred_caps, size, min,
+          max);
     gst_buffer_pool_config_add_option (config,
         GST_BUFFER_POOL_OPTION_VAAPI_VIDEO_META);
     if (!gst_buffer_pool_set_config (pool, config))

gst/vaapi/gstvaapipluginbase.h

@@ -213,7 +213,7 @@ gst_vaapi_plugin_base_propose_allocation (GstVaapiPluginBase * plugin,
 G_GNUC_INTERNAL
 gboolean
 gst_vaapi_plugin_base_decide_allocation (GstVaapiPluginBase * plugin,
-    GstQuery * query, guint feature);
+    GstQuery * query, guint feature, GstCaps *preferred_caps);
 
 G_GNUC_INTERNAL
 GstFlowReturn

gst/vaapi/gstvaapipostproc.c

@@ -1339,7 +1339,7 @@ static gboolean
 gst_vaapipostproc_decide_allocation (GstBaseTransform * trans, GstQuery * query)
 {
   return gst_vaapi_plugin_base_decide_allocation (GST_VAAPI_PLUGIN_BASE (trans),
-      query, 0);
+      query, 0, NULL);
 }
 
 static void