[Bug 747352] applemedia: texture cache negotiation doesn't work

[GStreamer (GNOME Bugzilla)]

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

--- Comment #12 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Good work :) I just looked over everything shortly, will take a longer look
tomorrow probably.

Just some comments

(In reply to Ilya Konstantinov from comment #9)
> Created attachment 300969 [details] [review]
> applemedia: Core Video NV12 textures
> 
> I couldn't test the NV12 patch since glimagesink doesn't accept NV12 over
> GLMemory. Any idea how I can test it?

glupload or glcolorconvert handle NV12 (unless broken)

(In reply to Ilya Konstantinov from comment #8)
> Created attachment 300968 [details] [review]
> avfvideosrc: Fix risky typo
> 
> LAST BUT NOT LEAST:
> 
> -  GST_BUFFER_OFFSET_END (*buf) = GST_BUFFER_OFFSET (buf) + 1;
> +  GST_BUFFER_OFFSET_END (*buf) = GST_BUFFER_OFFSET (*buf) + 1;
> 
> How did this ever work?!

It was just reading some random value from the stack there :) Good that nothing
really uses the offset/offset-end here ;)

(In reply to Ilya Konstantinov from comment #7)
> Created attachment 300967 [details] [review]
> glimagesink: Don't keep next_buffer after drawing
> 
> SMALL PATCH for simplifying our already-confusing buffer refcount:
> 
> next_buffer should be alive between prepare and show_frame.
> For keeping a stationary frame, we have aptly named named stored_buffer.

The reason for this is that we want to keep around the buffer until we draw the
next one. IIRC it's not guaranteed that after glTexImage2D() or similar
everything is done, and we can't safely unref/destroy/reuse the memory yet.

(In reply to Ilya Konstantinov from comment #6)
> NOTE: Xcode's OpenGL ES Analysis instrument shows further issues:
> 
> 1 x Uninitialized Texture Data
> 3 x Redundant Uniform Location Query
> 3 x Recommend Using Element Array Buffers
> 3 x Redundant Call

Does it also tell you where?

(In reply to Ilya Konstantinov from comment #11)
> BEFORE I FORGET:
> 
> With my patches, gst_glimage_sink_propose_allocation is only called for
> non-GLMemory cases. If so, should the following 2 lines be in it?
> 
>   if (glimage_sink->context->gl_vtable->FenceSync)
>     gst_query_add_allocation_meta (query, GST_GL_SYNC_META_API_TYPE, 0);

I think so. Someone should really review all that negotiation code, it seems a
bit half-refactored :)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.