Created attachment 371173 [details] [review]
jpegenc: Disallow upstream renegotiation if downstream doesn't support it

Currently when upstream tries to renegotiate the input format, we just
let it decide whatever it wants solely based on what jpegenc supports,
not taking into account what downstream can do.

This patch checks if downstream supports renegotiation by querying
downstream caps and checking if they are fixed, which
implies nothing else will be accepted.

Maybe in some cases we could be able to renegotiate some parts
of upstream format without changing downstream caps, but better safe
than sorry for now.

Comment on attachment 371173 [details] [review]
jpegenc: Disallow upstream renegotiation if downstream doesn't support it

Why is this necessary at all? The default getcaps should query downstream and then convert the caps accordingly (with the proxy getcaps function). This should have the same effect.

Review of attachment 371173 [details] [review]:

::: ext/jpeg/gstjpegenc.c
@@ +581,3 @@
+
+    downstream_caps = gst_pad_peer_query_caps (enc->srcpad, NULL);
+    if (gst_caps_is_fixed (downstream_caps)) {

image/jpeg are also fixed caps :)

@@ +587,3 @@
+
+      if (filter)
+        ret = gst_caps_intersect (input_caps, filter);

gst_caps_intersect_full(filter, input_caps, GST_CAPS_INTERSECT_FIRST)

(In reply to Sebastian Dröge (slomo) from comment #2)
> Comment on attachment 371173 [details] [review] [review]
> jpegenc: Disallow upstream renegotiation if downstream doesn't support it
> 
> Why is this necessary at all? The default getcaps should query downstream
> and then convert the caps accordingly (with the proxy getcaps function).
> This should have the same effect.

The format is not availaible downstream so the default implementation uses the first specified, which might lead to the not negotiated error.

(In reply to Sebastian Dröge (slomo) from comment #3)
> Review of attachment 371173 [details] [review] [review]:
> 
> ::: ext/jpeg/gstjpegenc.c
> @@ +581,3 @@
> +
> +    downstream_caps = gst_pad_peer_query_caps (enc->srcpad, NULL);
> +    if (gst_caps_is_fixed (downstream_caps)) {
> 
> image/jpeg are also fixed caps :)

That doesn't sound possible to have upstream fixed and downstream fixed without all the required fields.

> @@ +587,3 @@
> +
> +      if (filter)
> +        ret = gst_caps_intersect (input_caps, filter);
> 
> gst_caps_intersect_full(filter, input_caps, GST_CAPS_INTERSECT_FIRST)

Fixed.

Created attachment 371211 [details] [review]
jpegenc: Disallow upstream renegotiation if downstream doesn't support it

Currently when upstream tries to renegotiate the input format, we just
let it decide whatever it wants solely based on what jpegenc supports,
not taking into account what downstream can do.

This patch checks if downstream supports renegotiation by querying
downstream caps and checking if they are fixed, which
implies nothing else will be accepted.

Maybe in some cases we could be able to renegotiate some parts
of upstream format without changing downstream caps, but better safe
than sorry for now.

We should definatly fix renegotiation in this encoder, it is trivial case as it's key frame only.

This actually is the consequence of https://bugzilla.gnome.org/show_bug.cgi?id=795463