[PATCH] winegstreamer: Fixes for video flipping

[Jan Schmidt]

On 16/07/16 05:25, Andrew Eikum wrote:
> 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?

No, decodebin only handles decoding - it stops and outputs pads once it
successfully decodes raw video or audio.

>> 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?

OK

>> 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.

This is actually the part that fixes bug 40764 (although the refcounting
ones are needed to avoid crashes too). You can set them to PLAYING or
PAUSED - it doesn't actually matter. The only difference between playing
and paused states in GStreamer is in elements that do operations on the
clock, such as audio or video sinks. For videoflip and videoconvert the
important thing is that they aren't still in READY state by the time
they receive data, because then their pads will not have been activated.
That's the core of the race condition that's causing 40764.

>> @@ -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?

Those lines replace the insertion of videoflip with the insertion of
'videoconvert -> videoflip' as a linked pair and then treats them as a
single unit - getting the sink pad of the first element, and the source
pad of the videoflip.

The elements are owned by the bin afterward, so there's no need to keep
a ref or clean them up explictly. They'll be disposed of automatically
when the container is unreffed on cleanup.

Cheers,
Jan.
> Andrew
> 

-- 
Jan Schmidt, Centricular Ltd - http://centricular.com/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20160716/33250207/attachment.sig>