After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 747352 - applemedia: texture cache negotiation doesn't work
applemedia: texture cache negotiation doesn't work
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 1.5.1
Assigned To: Ilya Konstantinov
GStreamer Maintainers
Depends on:
Blocks: 747519
 
 
Reported: 2015-04-04 22:26 UTC by Ilya Konstantinov
Modified: 2015-04-23 22:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gl: Fix query_allocate for GLMemory (2.97 KB, patch)
2015-04-05 02:27 UTC, Ilya Konstantinov
reviewed Details | Review
applemedia: Core Video RGBA textures on iOS (8.01 KB, patch)
2015-04-05 02:52 UTC, Ilya Konstantinov
none Details | Review
glimagesink: Fix drawing on iOS (GLES) (1.38 KB, patch)
2015-04-05 02:57 UTC, Ilya Konstantinov
reviewed Details | Review
glimagesink: Don't keep next_buffer after drawing (2.11 KB, patch)
2015-04-05 02:59 UTC, Ilya Konstantinov
rejected Details | Review
avfvideosrc: Fix risky typo (889 bytes, patch)
2015-04-05 03:00 UTC, Ilya Konstantinov
committed Details | Review
applemedia: Core Video NV12 textures (2.55 KB, patch)
2015-04-05 03:01 UTC, Ilya Konstantinov
none Details | Review
applemedia: Core Video RGBA textures on iOS (8.01 KB, patch)
2015-04-05 03:11 UTC, Ilya Konstantinov
none Details | Review

Description Ilya Konstantinov 2015-04-04 22:26:49 UTC
For the following pipeline:

  avfvideosrc device-index=0 ! 
  video/x-raw(memory:GLMemory),width=1280,height=720,framerate=30/1 ! 
  glimagesink enable-last-sample=0 sync=false

the texture cache path doesn't work, i.e. textureCache == NULL and no optimisation takes place.

PRELIMINARY ANALYSIS

In avfvideosrc, we have the following code in decideAllocations:

  gst_query_find_allocation_meta (query, GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE, &idx)

This meta is only added by _upload_meta_upload_propose_allocation, which is called through:
  gst_gl_upload_propose_allocation
    _gst_gl_upload_element_propose_allocation
       GST_BASE_TRANSFORM (bt)->propose_allocation

However, gst_gl_base_filter_query contains:

  switch (GST_QUERY_TYPE (query)) {
    case GST_QUERY_ALLOCATION:
    {
      if (direction == GST_PAD_SINK
          && gst_base_transform_is_passthrough (trans)) {
        _find_local_gl_context (filter);

        return gst_pad_peer_query (GST_BASE_TRANSFORM_SRC_PAD (trans), query);
      }
      break;
    }

Since the direction of the GstUpload "filter" (why is it a filter?) is indeed -> sink, the query is forwarded onward to gstglimagesink, which lacks the special handling.
Comment 1 Ilya Konstantinov 2015-04-04 22:48:38 UTC
I've realized it's not glupload's job, so I've been barking up the wrong tree.

However, c06715bde6b261d3f93381a0bdb57cab0e35c647 from 2015-02-03 seems interesting:

    glupload/download/convert: provide transform_caps functions
    
    Allows finer grain decisions about formats and features at each
    stage of the pipeline.
    Also provide propose_allocation for glupload besed on the supported
    methods.

This commit removed the GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE meta addition from gst_glimage_sink_propose_allocation.
Comment 2 Ilya Konstantinov 2015-04-04 22:53:58 UTC
... but this commit added to gst_glimage_sink_propose_allocation:

+  gst_gl_upload_propose_allocation (glimage_sink->upload, NULL, query);

But then 8a0017e21d5f9a8507f0593c6b24f723aa415258 from 2015-02-20 removed it:

    glimagesink: implement as a bin
    
    glupload ! glcolorconvert ! sink
    
    Some properties are manually forwarded.  The rest are available using
    GstChildProxy.
    
    The two signals are forwarded as well.
Comment 3 Sebastian Dröge (slomo) 2015-04-05 01:15:02 UTC
I think we should reconsider some things here, as I mentioned in bug #743974

But nonetheless the caps transformation functions should be fixed up. While glimagesink (currently) only seems to support GL memory, GLUpload can convert everything to GL memory... so it shouldn't just proxy the caps query, but also add all the things to the result it sends upstream that it can handle (raw memory, textureuploadmeta, eglimage, ...)
Comment 4 Ilya Konstantinov 2015-04-05 02:27:59 UTC
Created attachment 300964 [details] [review]
gl: Fix query_allocate for GLMemory

Commit 8a0017e21d5f9a8507f0593c6b24f723aa415258 meant for
GstGLUploadElement to propose allocation, but as a result
of a condition in GstGLBaseFilterClass, it didn't do so.

As a side effect, upstream elements couldn't enable their GL texture
support.

Note: This motivation for this patch is to fix glimagesink.
Comment 5 Ilya Konstantinov 2015-04-05 02:52:53 UTC
Created attachment 300965 [details] [review]
applemedia: Core Video RGBA textures on iOS

We were configuring RGBA textures; no wonder Core Video refused to
give us NV12 textures. Luckily for us, RGBA is what we wanted.
Comment 6 Ilya Konstantinov 2015-04-05 02:57:51 UTC
Created attachment 300966 [details] [review]
glimagesink: Fix drawing on iOS (GLES)

I'm not an OpenGL expert, just following the example from:
https://developer.apple.com/library/ios/samplecode/RosyWriter/Listings/Classes_RosyWriterOpenGLRenderer_m.html

See also:
http://stackoverflow.com/questions/21501510/trouble-rendering-camera-feed-as-opengl-texture-ios

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
Comment 7 Ilya Konstantinov 2015-04-05 02:59:36 UTC
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.
Comment 8 Ilya Konstantinov 2015-04-05 03:00:34 UTC
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?!
Comment 9 Ilya Konstantinov 2015-04-05 03:01:42 UTC
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?
Comment 10 Ilya Konstantinov 2015-04-05 03:11:44 UTC
Created attachment 300970 [details] [review]
applemedia: Core Video RGBA textures on iOS

Whoops!

Should output GL_BGRA, so that we'll draw GL_RGBA...

Otherwise, blue and red were swapped.

Whatevah.
Comment 11 Ilya Konstantinov 2015-04-05 03:17:36 UTC
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);
Comment 12 Sebastian Dröge (slomo) 2015-04-05 04:00:12 UTC
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] [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] [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] [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 :)
Comment 13 Ilya Konstantinov 2015-04-05 04:25:58 UTC
(In reply to Sebastian Dröge (slomo) from comment #12)
> > 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)

static GstCaps *
gst_glimage_sink_get_caps (GstBaseSink * bsink, GstCaps * filter)
{
  GstCaps *tmp = NULL;
...
  tmp = gst_caps_from_string ("video/x-raw(memory:GLMemory),format=RGBA");

Hmm. Could this be related?

> (In reply to Ilya Konstantinov from comment #7)
> > Created attachment 300967 [details] [review] [review] [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.

Yes, we still keep it -- it's called "stored_buffer". It's what we draw, and what we'll keep drawing until we get another buffer.

I thought "next_buffer" is supposed to be only alive between prepare and show_frame.

Before this patch, we ref the same buffer twice -- in 1) stored_buffer, and 2) next_buffer, and this is just confusing.

> (In reply to Ilya Konstantinov from comment #6)
> > NOTE: Xcode's OpenGL ES Analysis instrument shows further issues:
> > 
... 
> Does it also tell you where?

It pinpoints the exact GL calls, so you could grep and figure it out. I don't think it pinpoints lines of code, but I'll snoop some more.

> >   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 :)

I think Matthew Waters can answer best.
Comment 14 Nicolas Dufresne (ndufresne) 2015-04-05 15:54:25 UTC
Review of attachment 300967 [details] [review]:

This will cause tearing. We have no control on exactly when the texture will actually be used.
Comment 15 Ilya Konstantinov 2015-04-05 16:35:20 UTC
(In reply to Nicolas Dufresne (stormer) from comment #14)
> Review of attachment 300967 [details] [review] [review]:
> 
> This will cause tearing. We have no control on exactly when the texture will
> actually be used.

The texture that's used in the drawing routine is glimage_sink->stored_buffer. I'm merely nullifying next_buffer earlier (and unrefing once), not changing any behavior.
Comment 16 Matthew Waters (ystreet00) 2015-04-05 20:51:34 UTC
(In reply to Ilya Konstantinov from comment #15)
> (In reply to Nicolas Dufresne (stormer) from comment #14)
> > Review of attachment 300967 [details] [review] [review] [review]:
> > 
> > This will cause tearing. We have no control on exactly when the texture will
> > actually be used.
> 
> The texture that's used in the drawing routine is
> glimage_sink->stored_buffer. I'm merely nullifying next_buffer earlier (and
> unrefing once), not changing any behavior.

Removing the ref will cause flickering and possible reading invalid textures.  An extensive reason as to why it's needed is available in
5b8d7a443ead269d034401a6788f2d47f8fa4bc5 glupload: provide the output buffer that is rendered into.
Comment 17 Ilya Konstantinov 2015-04-05 22:46:11 UTC
(In reply to Matthew Waters from comment #16)
> Removing the ref will cause flickering and possible reading invalid
> textures.  An extensive reason as to why it's needed is available in
> 5b8d7a443ead269d034401a6788f2d47f8fa4bc5 glupload: provide the output buffer
> that is rendered into.

OK, I think I can see the problem. Sorry for the trouble -- this patch doesn't change much anyway, so I'm retracting this part.
Comment 18 Matthew Waters (ystreet00) 2015-04-06 00:07:25 UTC
For the original problem, it is not going to be guaranteed that the upload meta is present when you configure with the GLMemory caps features.  Especially if we selectively enable/advertise meta's/allocators based on the caps features (which I have planned).

avfvideosrc needs to check based on the GLMemory caps features if it needs to use GLMmeory or not rather than on the existence of an unrelated meta that will not be used.
Comment 19 Matthew Waters (ystreet00) 2015-04-06 00:09:29 UTC
Review of attachment 300964 [details] [review]:

I don't think this is needed if you fix avfvideosrc to not detect GL downstream with the upload meta.
Comment 20 Matthew Waters (ystreet00) 2015-04-06 00:12:41 UTC
Review of attachment 300966 [details] [review]:

This should be taken care of at texture creation time.  Overriding the sampling parameters of the texture is not friendly to other texture types which may have different constraints.  I'm also not sure if its necessary for the textures CoreVideo provides.
Comment 21 Ilya Konstantinov 2015-04-06 00:51:33 UTC
(In reply to Matthew Waters from comment #18)
> For the original problem, it is not going to be guaranteed that the upload
> meta is present when you configure with the GLMemory caps features. 
> Especially if we selectively enable/advertise meta's/allocators based on the
> caps features (which I have planned).

The one who broke compatibility should've done it. They could've grepped the code for 
GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE to see what effect this removal had, at the very least on elements in the same repo.

(In reply to Matthew Waters from comment #19)
> I don't think this is needed if you fix avfvideosrc to not detect GL
> downstream with the upload meta.

Right now nothing calls the propose_allocation of gluploadelement, the propose_allocation that you and others have painstakingly written.

(In reply to Matthew Waters from comment #20)
> I'm also not sure if its necessary for the textures CoreVideo provides.

Right now, there's black screen on iOS without it, and good smooth video with it. You're welcome to find another solution, but I probably wouldn't be putting more time into it.
Comment 22 Matthew Waters (ystreet00) 2015-04-06 02:03:41 UTC
> (In reply to Matthew Waters from comment #19)
> > I don't think this is needed if you fix avfvideosrc to not detect GL
> > downstream with the upload meta.
> 
> Right now nothing calls the propose_allocation of gluploadelement, the
> propose_allocation that you and others have painstakingly written.

Which is correct as glupload is not doing anything as it's in passthrough mode.

> (In reply to Matthew Waters from comment #20)
> > I'm also not sure if its necessary for the textures CoreVideo provides.
> 
> Right now, there's black screen on iOS without it, and good smooth video
> with it. You're welcome to find another solution, but I probably wouldn't be
> putting more time into it.

i.e. don't do glTexParameters in glimagesink but when you create/wrap the texture from CoreVideo.
Comment 23 Ilya Konstantinov 2015-04-08 15:24:37 UTC
Alessandro, CC'ing you cause you're the last person to touch that.

I've attached patches for RGBA texture export. In your code you've had NV12 textures hardcoded even in RGBA mode, which is a bug.

I proposed a fix, but I'll let more experienced people than me take over this now.
Comment 24 Ilya Konstantinov 2015-04-11 04:45:11 UTC
(In reply to Matthew Waters from comment #18)
> avfvideosrc needs to check based on the GLMemory caps features if it needs
> to use GLMmeory or not rather than on the existence of an unrelated meta
> that will not be used.

So I assume something like this?

https://github.com/ikonst/gst-plugins-bad/commit/953e89763af12ee9ced80eabdd9145cd7be5a636
Comment 25 Ilya Konstantinov 2015-04-11 16:09:36 UTC
Nicolas, I've looked again at Matthew's code and mine, and tried to analyse the ref-unrefs:

MATTHEW'S CODE AFTER 5b8d7a443ead269d034401a6788f2d47f8fa4bc5

prepare:
  next = B1
  stored = 0
  B1+

show-frame-pre-redisplay:
  next = B1
  stored = B1
  B1++

show-frame-post-redisplay:
  next = B1
  stored = B1
  B1++

prepare:
  next = B2
  stored = B1
  B1+
  B2+

show-frame-pre-redisplay:
  next = B2
  stored = B2
  B1+
  B2++

show-frame-post-redisplay:
  next = B1
  stored = B1
  B1 <- unref'd
  B2++

etc.

ILYA'S PROPOSED CHANGE:

prepare:
  next = B1
  stored = 0
  B1+

show-frame-pre-redisplay:
  next = B1
  stored = B1
  B1++

[redisplay]

show-frame-post-redisplay:
  next = 0         <-- DIFFERENCE
  stored = B1
  B1+              <-- DIFFERENCE

prepare:
  next = B2
  stored = B1
  B1+
  B2+

show-frame-pre-redisplay:
  next = B2
  stored = B2
  B1+
  B2++

[redisplay]

show-frame-post-redisplay:
  next = 0        <-- DIFFERENCE
  stored = B1
  B1 <- unref'd
  B2+
Comment 26 Alessandro Decina 2015-04-13 04:50:18 UTC
(In reply to Ilya Konstantinov from comment #23)
> Alessandro, CC'ing you cause you're the last person to touch that.

Hi, sorry it took me a while to reply.


> I've attached patches for RGBA texture export. In your code you've had NV12
> textures hardcoded even in RGBA mode, which is a bug.
> 
> I proposed a fix, but I'll let more experienced people than me take over
> this now.

It's actually not a bug. When you negotiate RGBA with AVF it internally produces NV12 and then converts to RGBA. The conversion is slower than ours according to my profiling, so we get NV12 and then convert to RGBA ourselves with GstGLColorConvert.
Comment 27 Ilya Konstantinov 2015-04-23 19:52:32 UTC
Alessandro's commit edf9035d02c947ed9c836ddafa01fa34681918b4 seems to fix this.