Thanks very much for working on this, I'm glad to have more eyes looking at it. Comments in-line below.
On Sat, Jul 16, 2016 at 02:51:10AM +1000, Jan Schmidt wrote:
The videoflip element doesn't handle all formats, so some formats like Intel Indeo 3 will fail. Add videoconvert to do format conversion when it's needed.
Shouldn't the decodebin handle this during format negotiation when the videoflip is connected to it?
Fix some refcount issues too - add a new element to a bin takes ownership of the floating reference, so there's no need to keep pointers around to unref later.
Good catch. Could you separate this out into a different, stand-alone patch from the other changes?
Fix state setting of new elements - always just set them to PAUSED. gst_element_sync_state_with_parent() can be problematic, and we only need these in paused or playing anyway.
I'm not sure about this part. It makes sense to me for all members of a container to be in the same state. Why is it better for these to be PAUSED while others are PLAYING? If it's needed, this should be its own patch, too.
@@ -846,13 +855,16 @@ static void init_new_decoded_pad(GstElement *bin, GstPad *pad, GSTImpl *This)
gst_util_set_object_arg(G_OBJECT(pin->flipfilter), "method", "vertical-flip");
gst_bin_add(GST_BIN(This->container), pin->flipfilter);gst_element_sync_state_with_parent(pin->flipfilter);
gst_element_set_state (vconv, GST_STATE_PAUSED);gst_bin_add(GST_BIN(This->container), vconv); /* bin takes ownership */gst_element_set_state (pin->flipfilter, GST_STATE_PAUSED);gst_bin_add(GST_BIN(This->container), pin->flipfilter); /* bin takes ownership */
pin->flip_sink = gst_element_get_static_pad(pin->flipfilter, "sink");
gst_element_link (vconv, pin->flipfilter);pin->flip_sink = gst_element_get_static_pad(vconv, "sink"); if(!pin->flip_sink){ WARN("Couldn't find sink on flip filter\n");
gst_object_unref(pin->flipfilter);
I believe this leaves a dangling element in the container. Should these lines be replaced with calls to gst_bin_remove() instead?
Andrew