GNOME Bugzilla – Bug 667222
dvdspu: use refcounting instead of explicit copy
Last modified: 2015-06-23 13:14:46 UTC
in most cases would amount to the same thing, but all the same there is no reason to force a copy patch attached
Created attachment 204529 [details] [review] dvdspu: use refcnting instead of explicit copy
The comment makes me think this is intentional: /* Take a copy in case we hit a still frame and need the pristine * frame around */ - copy = gst_buffer_copy (buf); + copy = gst_buffer_ref (buf);
yeah, I saw that comment too.. but was unsure about the scenario where this would need to be a copy. I think maybe to avoid buffer exhaustion in case of a vsink element that can only allocate a fixed number of buffers. And even in that case the vsink should fall back to malloc'd buffers and dropping frames or doing a copy internally. So I still think it is better to use refcnt'ing. And if using a copy is a workaround for some specific video sink, then that should be fixed separately in the video sink.
I think I made it take a copy rather than store a reference to the sink allocated buffer when dvdspu is about to draw an overlay. Storing a ref, just makes it do a copy in gst_buffer_make_writable, which would then make the downstream buffer be a normal memory buffer, and force a copy in the sink in the common case. This way, the copy in the sink happens in the uncommon case of duplicating a reference frame for a DVD still sequence. In the case of no dvdspu overlay, it just pushes the buffer downstream and stores a ref rather than a copy. Another fix might be to actually make dvdspu allocate downstream when it needs to dup a frame for writability.
I think I also thought at one point about making the render functions do the dup'ing as they go if necessary: They can dup across several lines of the frame into the destination buffer, then render the overlay onto them which saves some cache thrashing.
Jan, any more thoughts on this? WONTFIX?
One way or another, when directly drawing an subpicture onto the frame, taking a copy is unavoidable - might as well make it explicit. When deferring rendering downstream with a GstVideoOverlayCompositionMeta, it could just take the ref, but we haven't merged that.