(In reply to comment #0)
> Unfortunately, there is no way to negotiate this properly. We could set the
> format in the caps to be RGB, but that is wrong because if you map the buffer
> before uploading it, the data is not necessarily RGB. Another way would be a
> flag in the UploadMeta, but 1) there is no padding to add that in the structure
> and 2) the sink should ideally know this information early (during caps
> negotiation probably) in order to initialize its internal state before the
> first buffer arrives. Maybe the best would be a field in the caps... something
> like "uploaded-format=(string)xRGB" ?

I don't think this is true. I think this only means that if you have negotiated the UploadMeta caps feature, the corresponding negotiated format shall be in RGB, for sysmem negotiation, it should  be YUV.

I'm leaving to needinfo, but let us know why you can't negotiate with caps features.

(In reply to comment #1)
> I don't think this is true. I think this only means that if you have negotiated
> the UploadMeta caps feature, the corresponding negotiated format shall be in
> RGB, for sysmem negotiation, it should  be YUV.

As I said, I could do that (i.e. negotiate "video/x-raw(meta:GstVideoGLTextureUploadMeta), format=RGBA"), but it feels wrong because if any intermediate plugin tries for whatever reason to map the buffer and read its data, it will read data in a different format. It feels more natural to negotiate: "video/x-raw(meta:GstVideoGLTextureUploadMeta), format=I420, uploaded-format=RGBA"

> I'm leaving to needinfo, but let us know why you can't negotiate with caps
> features.

You mean add another caps feature?

(In reply to comment #2)
> (In reply to comment #1)
> > I don't think this is true. I think this only means that if you have negotiated
> > the UploadMeta caps feature, the corresponding negotiated format shall be in
> > RGB, for sysmem negotiation, it should  be YUV.
> 
> As I said, I could do that (i.e. negotiate
> "video/x-raw(meta:GstVideoGLTextureUploadMeta), format=RGBA"), but it feels
> wrong because if any intermediate plugin tries for whatever reason to map the
> buffer and read its data, it will read data in a different format. It feels
> more natural to negotiate: "video/x-raw(meta:GstVideoGLTextureUploadMeta),
> format=I420, uploaded-format=RGBA"

If said intermediate plugin does not support the GstVideoGLTextureUploadMeta, then you negotiate sysmem (or fail).  I can't think of a case for supporting mapping buffers and using upload meta by the same element.

> > I'm leaving to needinfo, but let us know why you can't negotiate with caps
> > features.
> 
> You mean add another caps feature?

Negotiate between using GstVideoGLTextureUploadMeta or not.

> If said intermediate plugin does not support the GstVideoGLTextureUploadMeta,
> then you negotiate sysmem (or fail).  I can't think of a case for supporting
> mapping buffers and using upload meta by the same element.

What if a tee element is used, one branch goes to a video sink which supports that meta, and another branch goes to deinterlacer+videoresize+rtp payloader+udpsink elements for example?

Changing the format in the caps just because a meta is added just feels wrong to me. The caps should describe what is really inside. Additional mechanisms like this meta are placed on top of that. They do not replace anything.

(In reply to comment #4)
> > If said intermediate plugin does not support the GstVideoGLTextureUploadMeta,
> > then you negotiate sysmem (or fail).  I can't think of a case for supporting
> > mapping buffers and using upload meta by the same element.
> 
> What if a tee element is used, one branch goes to a video sink which supports
> that meta, and another branch goes to deinterlacer+videoresize+rtp
> payloader+udpsink elements for example?

Then negotiation will fail based on caps features.  Either both sides of the tee will support the meta or will not.

> Changing the format in the caps just because a meta is added just feels wrong
> to me. The caps should describe what is really inside. Additional mechanisms
> like this meta are placed on top of that. They do not replace anything.

Also, there is no guarantee that mapping the buffer is supported when the meta exists, I would even suggest that mapping a buffer with the meta would be undefined behaviour.  The meta provides a way to get the data from a buffer to some GL texture/s of a specified format without downstream explicitly mapping the buffer.

(In reply to comment #5)
> Then negotiation will fail based on caps features.  Either both sides of the
> tee will support the meta or will not.

Which means that for every such meta, all of these elements (deinterlacer etc.) have to be rewritten. Either that, or some kind of converter element, which only complicates things and is actually unnecessary. Such a meta should never rule out other features.

> > Changing the format in the caps just because a meta is added just feels wrong
> > to me. The caps should describe what is really inside. Additional mechanisms
> > like this meta are placed on top of that. They do not replace anything.
> 
> Also, there is no guarantee that mapping the buffer is supported when the meta
> exists, I would even suggest that mapping a buffer with the meta would be
> undefined behaviour.  The meta provides a way to get the data from a buffer to
> some GL texture/s of a specified format without downstream explicitly mapping
> the buffer.

There is a guarantee - if the SystemMemory caps feature is present, then it can be mapped. Also, if a GstMemory inside the GstBuffer cannot be mapped (= has the GST_MEMORY_FLAG_NOT_MAPPABLE flag set), then mapping will not be possible.

(In reply to comment #6)
> (In reply to comment #5)
> > Then negotiation will fail based on caps features.  Either both sides of the
> > tee will support the meta or will not.
> 
> Which means that for every such meta, all of these elements (deinterlacer etc.)
> have to be rewritten. Either that, or some kind of converter element, which
> only complicates things and is actually unnecessary. Such a meta should never
> rule out other features.

Why do they have to be rewritten?  I would argue that if you have the meta (and only that meta) in the caps features, then that's the only way of accessing the data.  It's not ruling out other features because they weren't there to begin with.  If however, you also have the Sysmem capsfeatures on the caps then you can map the buffer and do what you like however (and this is what the bug is about), the format should be the same for both the uploaded meta and accessing the buffer through mapping.  Otherwise, you can negotiate between upload meta and sysmem separately without any issues.

> > > Changing the format in the caps just because a meta is added just feels wrong
> > > to me. The caps should describe what is really inside. Additional mechanisms
> > > like this meta are placed on top of that. They do not replace anything.
> > 
> > Also, there is no guarantee that mapping the buffer is supported when the meta
> > exists, I would even suggest that mapping a buffer with the meta would be
> > undefined behaviour.  The meta provides a way to get the data from a buffer to
> > some GL texture/s of a specified format without downstream explicitly mapping
> > the buffer.
>
> There is a guarantee - if the SystemMemory caps feature is present, then it can
> be mapped. Also, if a GstMemory inside the GstBuffer cannot be mapped (= has
> the GST_MEMORY_FLAG_NOT_MAPPABLE flag set), then mapping will not be possible.

It can only be mapped if Sysmem is negotiated. Caps features are all or nothing.  i.e. you cannot mix and match caps features from different caps structures.  Either you support all the caps features in a certain caps structure or you don't.  With that in mind, you could try to negotiate both Sysmem and UploadMeta however you will not be able to link with elements that support one or the other (but not both at the same time).

Sure, you may be able to map a buffer that's been negotiated as having upload meta (no sysmem), however the data could be in a completely different format to the one advertised in the caps, thus undefined behaviour.

If the meta is on a buffer but not in the caps as capsfeatures, this means that the memory can be used like normal system memory too. So there's indeed something missing here if the buffer content describes a different format depending on whether you use the meta or access it directly. Maybe needs another field in the meta to set the format, but how could that then be negotiated? We can't just add another field to the video/x-raw caps.


Independent of that, if you have multiple capsfeatures in a caps they are AND combined. That is downstream must support *all* of the features. When negotiating the elements need to decide on a set of features that is supported by everything.

I'm not sure I'm very keen in supporting buffer being multi-format. My general idea with that, is that you should negotiate one format and stick to it. If you have a tee, then to get GLUpload you'd need all side to support it. Obviously, it would fail if you add pads to the tee later and you don't handle reconfigure. From my perspective, negotiating GLUpload through caps is accepting not to map buffers because the performance will be likely slower then pure SW path if you do so. 

I'm thinking so also because the capsfeature produce an explosion in term of combinations, hence I'd prefer if we scope it down. To me, it's better to focus on getting 1 format per pad and that's it.

There was a comment about deinterlace, textoverlay, colorbalance, rewriting, etc. There is already some work going that direction. And for the case an element can't passthrough by meta, it should then prevent any similar meta, to avoid applying let's say deinterlacing after the text has been composed, and other non-sense.

Remains the case where upload meta is present, but not negotiated. In this case, I'm keen in adding API to the Meta (no field). Basically, that meta in this case is descriptive, hence no need to negotiate it.

So, my suggestion would be to add (optional) texture caps in the meta. We have basically two options, add fixes caps and that's it or add unfixed caps, with a get_current_caps() and set_caps() method. As it is descriptive, all elements using the meta this way, should be able to fallback using the sysmem mode and the format described in the pipeline caps (should be all right for glvideosink, eglglessink and cluttersink).

Yes, adding extra API to the meta sounds like a good option. However, I would retrieve a GstVideoInfo instead, since it can contain additional information like plane offsets and stride. But why set_caps() ? When would I want to use it? I would only add something like get_gl_video_info() .

As Sebastian reminded me that gst_query_add_allocation_meta() has a params
structure. Downstream could expose the formats it supports, in a way that if
adding UploadMeta requires resources, the producer could avoid it by comparing
the supported format against downstream supported formats. The idea again is
that you have the information to make decision, but not having the UploadMeta
in that descriptive case is never fatal.

(In reply to comment #11)
> Yes, adding extra API to the meta sounds like a good option. However, I would
> retrieve a GstVideoInfo instead, since it can contain additional information
> like plane offsets and stride. But why set_caps() ? When would I want to use
> it? I would only add something like get_gl_video_info() .

VideoInfo stride/offset/size is for mappable memory. When using the upload meta, you should not need that information, since the actual upload (often not really an upload) is done by the buffer owner/producer.

The suggestion for the get_caps()/set_caps() is to handle the case where there is multiple format to which you can upload (think ARGB, BGRA, RGBA, RGB565, etc.). Though, if using the extra params in the allocation query, I'd say just add a GstVideoFormat, full caps would be overkill. The params would have a set of GstVideoFormat to choose from, if not provided, it would mean the only valid format is the one in the negotiated caps.

But I think to go further it would be easier if someone provide some code that show the proposed API changes.

I do not follow. Why does downstream need to know anything? The reported issue is about an *internal* behavior of a GLUploadMeta implementation.

Lets say a plugin specifically written for hardware which supports hardwired color format conversion in the GPU. In that case, I would expect the following to happen:

1. Upstream creates GstBuffers with this GLmeta.

2. An element (lets say a sink) which supports this meta looks at it, and gets a texture from it. It also calls something like get_gl_video_info() to see how it should interpret that texture. In the hardwired GPU case, that video info contains RGB as video format, even though the GstBuffer caps are set to I420 (the GPU can do the I420->RGB conversion by itself). The sink now knows everything it needs to know - the data is to be interpreted as RGB pixels, so it just uses a dummy shader that passes through pixels.

3. In another case, a buffer is passed around, but it has a format that cannot be converted by the hardwired GPU conversion. In that case, get_gl_video_info() returns the same format as specified in the GstBuffer caps. The sink now knows that it needs to use a shader to convert the pixel format to RGB.

4. In yet another case (for example, when branching with tee), an element which does not care about the meta maps the buffer. Everything is as usual, nothing behaves differently.

I do not see how some set_caps() functions and queries are relevant here. This is *not* about allocators or buffer pools.

> VideoInfo stride/offset/size is for mappable memory. When using the upload
> meta, you should not need that information, since the actual upload (often not
> really an upload) is done by the buffer owner/producer.

You do need it if you want to be able to use the buffer contents as they are. This is particularly relevant for buffers which use physically contiguous memory that can be used as a backing store for buffers directly. I had this issue when trying to render HW-decoded video frames with the Vivante GPU direct texture extension. I added the number of padding rows to the height of the frame, and used that as the texture height. Then, in the shader, I modified the UV coordinates so that the additional padding rows are not rendered. The problem is that the extension did not allow for specifying padding explicitely, and expected I420 planes to be placed one after the other directly.

(In reply to comment #13)
> But I think to go further it would be easier if someone provide some code that
> show the proposed API changes.

I guess I could do that, as I am working on getting imxvpudec & coglsink working with the UploadMeta. I have to understand your proposal first, though...

(In reply to comment #14)
> I do not see how some set_caps() functions and queries are relevant here. This
> is *not* about allocators or buffer pools.

Metas are negotiated in the allocation query (not to confuse with allocators and buffer pool). This happens after the buffer caps has been negotiated. The idea is that the upstream element, that adds the TextureUploadMeta, may be able to upload to multiple texture format. Downstream, which will provide the textures, is most likely able to create texture for multiple formats too. The format the uploader uses and the format the texture is set for need to match. This means, information need to be shared from upstream (uploader) and downstream (that prepare the destination texture) in order to make it work.

The allocation_query() approach is nice, because if there is no match, there is no need to add UploadMeta on buffers. On certain stack, adding the UploadMeta and not using it has certain cost. On other stack, it's the only way to display the buffers, in this case it has to be in the caps negotiation.

> 
> > VideoInfo stride/offset/size is for mappable memory. When using the upload
> > meta, you should not need that information, since the actual upload (often not
> > really an upload) is done by the buffer owner/producer.
> 
> You do need it if you want to be able to use the buffer contents as they are.
> This is particularly relevant for buffers which use physically contiguous
> memory that can be used as a backing store for buffers directly. I had this
> issue when trying to render HW-decoded video frames with the Vivante GPU direct
> texture extension. I added the number of padding rows to the height of the
> frame, and used that as the texture height. Then, in the shader, I modified the
> UV coordinates so that the additional padding rows are not rendered. The
> problem is that the extension did not allow for specifying padding explicitely,
> and expected I420 planes to be placed one after the other directly.

This information is already in the buffer and the VideoMeta. All you had to do is read it. When implementing that meta, you can access buffer and user_data within the UploadMeta, it's private, but private in the sense that only implementer should use it. We could improve the doc though. Next tim you think your blocked, feel free to ask on IRC or mailing list.


Back to the subject:

In the display sink that support GlUpload, I'd suggest (and it should be optionnal) to define a GstStructure format to describe the supported formats.

See API gst_query_add_allocation_meta().

This can then be read in the allocation query (in decide_allocation). From this information, your element will decide to attach, or not, the TextureUploadMeta (making sure all the information it needs is set, on the buffer, the memory, the VideoMeta, or through the user_data).

In the UploadMeta, I would add a new member, let's say GstVideoFormat format, to let the display element know how to allocate the texture. I suspect doing it this way is a little more work, as you endup creating the texture after having received the first buffer (unless a unique format is supported). But I don't seen any better approach for now.

Unlike the capsfeature method, you must support the format describe in the buffer, otherwise negotiation will fail.

That is my proposed plan, even though this is just a suggestion, more thinking may bring up issues here.

Another note here, this description is all very tied to upstream allocation, which I think is the case here. Dowstream allocation is a bit of a case by case thing, where the only case we have yet is the EGL allocator, if you need a reference.

Here is an initial implementation with gstreamer-imx and coglsink:

http://cgit.collabora.com/git/user/gkiagia/gst-plugins-base.git/log/?h=gluploadmeta
http://cgit.collabora.com/git/user/gkiagia/gstreamer-imx.git/log/?h=gluploadmeta
http://cgit.collabora.com/git/user/gkiagia/cogl.git/log/?h=gluploadmeta

Initially I had a check in gstreamer-imx that would only add the meta if downstream claimed it is supported (through the allocation query), but as Sjoerd pointed out on irc, it sounds a better idea to always add it and let the sink decide whether it can use it or not, because if we have tees in the middle, it is going to get tricky to decide whether downstream supports it and we may end up not adding it while some sink can use it. I still haven't patched coglsink to *not* use the meta if the uploaded format is not supported.

(In reply to comment #18)
> http://cgit.collabora.com/git/user/gkiagia/gst-plugins-base.git/log/?h=gluploadmeta

I'd prefer simply ->format rather then uploaded_format. Also, for backward compatiblity, you need to handle the case this is set to GST_VIDEO_FORMAT_UNKNOWN (zero) in your implementation and need to be documented.

> http://cgit.collabora.com/git/user/gkiagia/cogl.git/log/?h=gluploadmeta

Saying you have texture support for GST_VIDEO_FORMATS_ALL is most likely not true. It should instead list the item you have in your switch, which I suppose match BASE_SINK_CAPS.

Implementation of GstVideoGLTextureUploadMetaParams looks wrong to me and isn't documented anywhere.

(In reply to comment #19)
> (In reply to comment #18)
> > http://cgit.collabora.com/git/user/gkiagia/gst-plugins-base.git/log/?h=gluploadmeta
> 
> I'd prefer simply ->format rather then uploaded_format. Also, for backward

ok

> compatiblity, you need to handle the case this is set to
> GST_VIDEO_FORMAT_UNKNOWN (zero) in your implementation and need to be
> documented.

I do handle the case where it is set to UNKNOWN and it is documented in gstvideometa.h

> > http://cgit.collabora.com/git/user/gkiagia/cogl.git/log/?h=gluploadmeta
> 
> Saying you have texture support for GST_VIDEO_FORMATS_ALL is most likely not
> true. It should instead list the item you have in your switch, which I suppose
> match BASE_SINK_CAPS.

This is a good argument. I was thinking that there can be 2 cases for the caps which have meta:GstVideoGLTextureUploadMeta:

1) We declare that the "format" field of those caps represents the "uploaded" format of the data, so in coglsink we advertise:

video/x-raw, format={ AYUV, YV12, I420, RGBA, BGRA, RGB, BGR, NV12 };
video/x-raw(meta:GstVideoGLTextureUploadFormat), format={ AYUV, YV12, I420, RGBA, BGRA, RGB, BGR, NV12 }

and when it gets negotiated with imxvpudec using the caps with the meta, format is the uploaded format (RGBA), although the buffers are actually I420:

video/x-raw(meta:GstVideoGLTextureUploadFormat), format=RGBA

2) We declare that the "format" field of those caps represents the actual buffer format and we only negotiate the uploaded format using the allocation query. Therefore coglsink advertises:

video/x-raw, format={ AYUV, YV12, I420, RGBA, BGRA, RGB, BGR, NV12 };
video/x-raw(meta:GstVideoGLTextureUploadFormat), format={ ANY }

and gets negotiated with imxvpudec using:

video/x-raw(meta:GstVideoGLTextureUploadFormat), format=I420

What I have implemented here is the second, because it looks cleaner. You only deal with the logic of the uploaded format negotiation in one place and you keep the information about what format the buffers actually are.

> Implementation of GstVideoGLTextureUploadMetaParams looks wrong to me and isn't
> documented anywhere.

Why is it wrong? It needs to be documented, though, that's true.

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > http://cgit.collabora.com/git/user/gkiagia/gst-plugins-base.git/log/?h=gluploadmeta
> > 
> > I'd prefer simply ->format rather then uploaded_format. Also, for backward
> 
> ok
> 
> > compatiblity, you need to handle the case this is set to
> > GST_VIDEO_FORMAT_UNKNOWN (zero) in your implementation and need to be
> > documented.
> 
> I do handle the case where it is set to UNKNOWN and it is documented in
> gstvideometa.h
> 
> > > http://cgit.collabora.com/git/user/gkiagia/cogl.git/log/?h=gluploadmeta

I meant this need to be documented somewhere in docs/design of -base, or in the Meta doc. Nobody implementing it's own sink will read cogl doc.

> > 
> > Saying you have texture support for GST_VIDEO_FORMATS_ALL is most likely not
> > true. It should instead list the item you have in your switch, which I suppose
> > match BASE_SINK_CAPS.
> 
> This is a good argument. I was thinking that there can be 2 cases for the caps
> which have meta:GstVideoGLTextureUploadMeta:
> 
> 1) We declare that the "format" field of those caps represents the "uploaded"
> format of the data, so in coglsink we advertise:
> 
> video/x-raw, format={ AYUV, YV12, I420, RGBA, BGRA, RGB, BGR, NV12 };
> video/x-raw(meta:GstVideoGLTextureUploadFormat), format={ AYUV, YV12, I420,
> RGBA, BGRA, RGB, BGR, NV12 }

For video/x-raw(meta:GstVideoGLTextureUploadFormat) the format field means the texture format you will upload to. If you have negotiated that, you should not be using map() on the buffer at all.

For video/x-raw, the format means the buffer format (e.g. I420). You may still have a upload meta, hence the need to add a format in the meta.

> 
> > Implementation of GstVideoGLTextureUploadMetaParams looks wrong to me and isn't
> > documented anywhere.
> 
> Why is it wrong? It needs to be documented, though, that's true.

For that I'd suggest proper review, that will mean, split the gst-plugins-base part from the cogl and imx part. File bugs for each and attach patch.

(In reply to comment #14)
> > VideoInfo stride/offset/size is for mappable memory. When using the upload
> > meta, you should not need that information, since the actual upload (often not
> > really an upload) is done by the buffer owner/producer.
> 
> You do need it if you want to be able to use the buffer contents as they are.
> This is particularly relevant for buffers which use physically contiguous
> memory that can be used as a backing store for buffers directly. I had this
> issue when trying to render HW-decoded video frames with the Vivante GPU direct
> texture extension. I added the number of padding rows to the height of the
> frame, and used that as the texture height. Then, in the shader, I modified the
> UV coordinates so that the additional padding rows are not rendered. The
> problem is that the extension did not allow for specifying padding explicitely,
> and expected I420 planes to be placed one after the other directly.

I am stuck a bit with this problem now... What happens here with Vivante, in other words, is that the uploaded texture has bigger dimensions than the video (width = row stride & height = video height + memory alignment padding) and it needs to be cropped in order to see just the video and not the video together with the padding areas interpreted as video.

Initially I thought that I could do it with GstVideoCropMeta, but it's not possible because in the shaders all sizes are normalized and I need to know what the texture width/height actually are in order to calculate the fraction to crop. With CropMeta it ends up being crop_meta->width == video_meta->width and crop_meta->height == video_meta->height, so the fraction crop_width/video_width is 1.0 and nothing is cropped.

For this to work, I need to know what the actual texture width & height are, so I am thinking of adding those also in the UploadMeta. This is actually a limitation of OpenGL ES, because in desktop GL it is possible to find what the texture size is and you don't have to carry it around.

Created attachment 274448 [details] [review]
gstvideometa: add format field in GstVideoGLTextureUploadMeta

Created attachment 274449 [details] [review]
gstvideometa: add width/height in GstVideoGLTextureUploadMeta

Created attachment 274450 [details] [review]
gstvideometa: document better the format/width/height fields of GstVideoGLTextureUploadMeta

Created attachment 274451 [details] [review]
docs/design: document GstVideoGLTextureUploadMeta and the texture format negotiation

Sorry, I'm not very clear on the cropping issue. I do remember then in big GL we could query the texture size. What I'm not too clear is how is that going to solve your issue. Would it be possible to elaborate on that. We need to make sure this won't also be an issue for other stack, that this solution is not Vivante specific.

(In reply to comment #27)
> Sorry, I'm not very clear on the cropping issue. I do remember then in big GL
> we could query the texture size. What I'm not too clear is how is that going to
> solve your issue. Would it be possible to elaborate on that. We need to make
> sure this won't also be an issue for other stack, that this solution is not
> Vivante specific.

The main detail to keep in mind here is that the Vivante extension does not allow you to specify plane offsets, strides, padding etc. explicitely. It assumes width = stride*bytesperpixel, and that planes are tightly packed together, that is, after the last row of plane 1 comes the first row of plane 2 etc. it is a bit more flexible if you actually upload the pixels, but if you specify a physical address to a DMA buffer, you have zero freedom.

So, the trick was to include padding etc. into the texture width & height. The actual frame would then be a subregion of this texture.

I agree that we must make sure this does not become too Vivante specific. However, I have seen this limitation in several other APIs (that is, no fields for stride, plane offsets etc.) so I believe this is still generic enough.

Hmm, I was just looking at the upload code in cogl and I noticed something else...

GstVideoGLTextureUploadMeta has a n_textures variable, which specifies how many textures there are going to be. This normally aligns with how many planes there are for the given format, so for example in eglglessink if n_textures != n_planes, it errors out. I have added exactly the same check in coglsink and I don't see a reason why this assertion could not hold true, so it looks like n_textures duplicates information from the format info. 

Similarly, I am not sure what exactly one should do with the texture_type of the upload meta. It looks like it duplicates format info again.

Finally, I was wondering what should happen with the width/height of the other textures in case the upload meta produces more than one texture. At the moment in coglsink I am using GST_VIDEO_INFO_COMP_WIDTH (&video_info, i) for each plane to find what the texture size is for that plane, but this is obviously wrong with what vivante does with the texture size. And if I add just one width/height variable in the upload meta, then I don't know what the size of the other textures is...

All this makes me wonder... maybe the upload meta should have a full GstVideoInfo in there, or it should require an extra GstVideoMeta with a special id? And get rid of the rest duplicated information...?


Hehe, and (unrelated) I just also found this in glimagesink:

  gl_context =
      gst_structure_new ("GstVideoGLTextureUploadMeta", "gst.gl.GstGLContext",
      GST_GL_TYPE_CONTEXT, glimage_sink->context, "gst.gl.context.handle",
      G_TYPE_POINTER, handle, "gst.gl.context.type", G_TYPE_STRING, platform,
      "gst.gl.context.apis", G_TYPE_STRING, gl_apis, NULL);
  gst_query_add_allocation_meta (query,
      GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE, gl_context);

This means that my patches are wrong.

So basically there have been some arguments on irc which are not written here:

* We agreed that adding texture size information on the meta is probably ok. Nicolas argued, though, that one should avoid using the fragment shader to implement this cropping, due to 16-bit float constraints on some platforms that will lead to bad video quality.

* Nicolas argued that upstream should not add the upload meta on buffers if downstream does not claim support for all the features that upstream needs (i.e. the texture format being different than the caps format or the texture size being different than the video size), so that we have backwards compatibility with sinks that are not updated to handle this information. The problem however is that adding a tee before the video sink will drop the allocation query and there will be no upload meta added although it may be supported by the sink. Carlos argued here that we should probably do a GstVideoGLTextureUploadMeta2 in order to solve the compatibility problem.


After my argument in comment #29, and considering the above, I want to make a new proposal:

* Add indeed a new GstVideoGLTextureUploadMeta2 which will be defined as:

struct _GstVideoGLTextureUploadMeta2 {
  GstMeta       meta;

  GstVideoGLTextureOrientation texture_orientation;
  GstVideoInfo texture_info;
  GstStructure *features;

  /* <private> */
  GstBuffer *buffer;
  GstVideoGLTextureUpload upload;

  gpointer      user_data;
  GBoxedCopyFunc user_data_copy;
  GBoxedFreeFunc user_data_free;
};

* The number of textures *must* be equal to the number of planes of the video format set in texture_info.
* The texture GL format (GL_RGBA, GL_RGB, etc) can be determined from the plane's bit depth, as set in texture_info - I think the sink is not really interested in the format, it is interested in how the data is stored in the texture (8bit -> A, 16bit -> RG, 24bit -> RGB, 32bit -> RGBA / see also CoglTextureComponents for example)
* The size of each texture can be calculated using GST_VIDEO_INFO_COMP_WIDTH/HEIGHT

* The "features" structure should contain a list of additional features that the sink needs to enable in order to use the uploaded texture. These can be one of:

- texture_size_includes_stride (G_TYPE_BOOLEAN): TRUE if the texture size includes the paddings of the buffer (as in vivante) and the sink needs to display only the top-left subregion of the texture that equals the video size as negotiated with the caps
- <anything else that may come up later> :)

* Sinks should advertise through the allocation query what they support using a structure named "GstVideoGLTextureUploadMeta2" with the following well-known fields:

- format (list - G_TYPE_STRING): the list of texture formats that are supported
- orientation (list - G_TYPE_INT): the list of texture orientations (GstVideoGLTextureOrientation) that are supported
- feature.* : reserved namespace for special features that the sink can support; these should be the same as the fields of the "features" structure (see above), but prefixed with "feature.". For example: feature.texture_size_includes_stride (G_TYPE_BOOLEAN)
- gst.gl.* : whatever GstGL needs to use (see comment #29)

* Upstream elements that produce buffers should decide based on the allocation query how to best match what the sink supports, but they should add the upload meta anyway, whether the sink supports all the features they need or not. The features field of the upload meta should be populated with all the features that are *mandatory* in order for the upload to work properly and nothing else.

* Sinks that support the upload meta should look at the format and the features in the upload meta and decide themselves if they are able to use it or not. If there is a certain feature in the "features" field that the sink does not know about, then it *must not* use the upload meta.

The idea is that with the "features" field, all the sinks that are going to implement GstVideoGLTextureUploadMeta2 must implement a logic block that will iterate over all the fields in the "features" structure and will enable their handling. But if there is at least one field in "features" that the sink does not know how to handle, then it must fall back to not using the upload meta, assuming that this feature is required. This way we can later add more features and/or change the semantics by using fields of the "features" structure to indicate whatever we need without dropping backwards compatibility. This also solves the dreadful problem with the tee.

* Now in case we have GstVideoGLTextureUploadMeta2 negotiated as a GstCapsFeature, the negotiation should happen entirely with caps and the "features" structure of the upload meta can be ignored, although it should still be there for convenience. The fields of the allocation query structure should be distributed in caps as follows:

- format: becomes a field in the caps (which exists already in the video/x-raw caps, but it changes semantics here, describing the format of the texture instead of the buffers)
- orientation: becomes a field in the caps
- feature.*: become GstCapsFeatures that are advertised additionally to "meta:GstVideoGLTextureUploadMeta2". I propose then prefixing them with "meta:GstVideoGLTextureUploadMeta2:" instead of "feature." so that they become meta:GstVideoGLTextureUploadMeta2:* or something similar... Obviously the sink here will need to advertise all possible combinations of them in different caps structures. Unfortunately I see is no other way to ensure that they all exist in both upstream and downstream elements.
- gst.gl*: these should stay in the allocation query structure.

(In reply to comment #30)
> After my argument in comment #29, and considering the above, I want to make a
> new proposal:
> 
> * Add indeed a new GstVideoGLTextureUploadMeta2 which will be defined as:

I think we agreed that if we where to create a new meta, we would give it a better name. An example could be GstVideoGLTextureMapMeta.

> 
> struct _GstVideoGLTextureUploadMeta2 {
>   GstMeta       meta;
> 
>   GstVideoGLTextureOrientation texture_orientation;
>   GstVideoInfo texture_info;

Any rational for the texture_ prefix ?

>   GstStructure *features;
> 
>   /* <private> */
>   GstBuffer *buffer;
>   GstVideoGLTextureUpload upload;
> 
>   gpointer      user_data;
>   GBoxedCopyFunc user_data_copy;
>   GBoxedFreeFunc user_data_free;
> };
> 
> * The number of textures *must* be equal to the number of planes of the video
> format set in texture_info.

Why ? Won't that prevent few platforms from using it ?

> * The texture GL format (GL_RGBA, GL_RGB, etc) can be determined from the
> plane's bit depth, as set in texture_info - I think the sink is not really
> interested in the format, it is interested in how the data is stored in the
> texture (8bit -> A, 16bit -> RG, 24bit -> RGB, 32bit -> RGBA / see also
> CoglTextureComponents for example)

Though you need to guaranty to never try with a miss-matching depth.

> * The size of each texture can be calculated using
> GST_VIDEO_INFO_COMP_WIDTH/HEIGHT

The dimension in pixels, to clarify.

> 
> * The "features" structure should contain a list of additional features that
> the sink needs to enable in order to use the uploaded texture. These can be one
> of:
> 
> - texture_size_includes_stride (G_TYPE_BOOLEAN): TRUE if the texture size
> includes the paddings of the buffer (as in vivante) and the sink needs to
> display only the top-left subregion of the texture that equals the video size
> as negotiated with the caps
> - <anything else that may come up later> :)

That is not seems very elegant, maybe some more research is needed, better rationals.

> 
> * Sinks should advertise through the allocation query what they support using a
> structure named "GstVideoGLTextureUploadMeta2" with the following well-known
> fields:
> 
> - format (list - G_TYPE_STRING): the list of texture formats that are supported
> - orientation (list - G_TYPE_INT): the list of texture orientations
> (GstVideoGLTextureOrientation) that are supported
> - feature.* : reserved namespace for special features that the sink can
> support; these should be the same as the fields of the "features" structure
> (see above), but prefixed with "feature.". For example:
> feature.texture_size_includes_stride (G_TYPE_BOOLEAN)
> - gst.gl.* : whatever GstGL needs to use (see comment #29)

Looks good, maybe a special attention to ensure this can be intersected / subset, so we can have generic code to check if it's compatible ?

> 
> * Upstream elements that produce buffers should decide based on the allocation
> query how to best match what the sink supports, but they should add the upload
> meta anyway, whether the sink supports all the features they need or not. The
> features field of the upload meta should be populated with all the features
> that are *mandatory* in order for the upload to work properly and nothing else
> 
> * Sinks that support the upload meta should look at the format and the features
> in the upload meta and decide themselves if they are able to use it or not. If
> there is a certain feature in the "features" field that the sink does not know
> about, then it *must not* use the upload meta.
> 
> The idea is that with the "features" field, all the sinks that are going to
> implement GstVideoGLTextureUploadMeta2 must implement a logic block that will
> iterate over all the fields in the "features" structure and will enable their
> handling. But if there is at least one field in "features" that the sink does
> not know how to handle, then it must fall back to not using the upload meta,
> assuming that this feature is required. This way we can later add more features
> and/or change the semantics by using fields of the "features" structure to
> indicate whatever we need without dropping backwards compatibility. This also
> solves the dreadful problem with the tee.

That why I'd like to see a reuse of the caps intersection and subset features here (see gst_structure_is_subset(), gst_structure_can_intersect()).

> 
> * Now in case we have GstVideoGLTextureUploadMeta2 negotiated as a
> GstCapsFeature, the negotiation should happen entirely with caps and the
> "features" structure of the upload meta can be ignored, although it should
> still be there for convenience. The fields of the allocation query structure
> should be distributed in caps as follows:
> 
> - format: becomes a field in the caps (which exists already in the video/x-raw
> caps, but it changes semantics here, describing the format of the texture
> instead of the buffers)
> - orientation: becomes a field in the caps
> - feature.*: become GstCapsFeatures that are advertised additionally to
> "meta:GstVideoGLTextureUploadMeta2". I propose then prefixing them with
> "meta:GstVideoGLTextureUploadMeta2:" instead of "feature." so that they become
> meta:GstVideoGLTextureUploadMeta2:* or something similar... Obviously the sink
> here will need to advertise all possible combinations of them in different caps
> structures. Unfortunately I see is no other way to ensure that they all exist
> in both upstream and downstream elements.
> - gst.gl*: these should stay in the allocation query structure.

(In reply to comment #31)
> (In reply to comment #30)
> > After my argument in comment #29, and considering the above, I want to make a
> > new proposal:
> > 
> > * Add indeed a new GstVideoGLTextureUploadMeta2 which will be defined as:
> 
> I think we agreed that if we where to create a new meta, we would give it a
> better name. An example could be GstVideoGLTextureMapMeta.

Ah true, I remember something. Agreed.

> > 
> > struct _GstVideoGLTextureUploadMeta2 {
> >   GstMeta       meta;
> > 
> >   GstVideoGLTextureOrientation texture_orientation;
> >   GstVideoInfo texture_info;
> 
> Any rational for the texture_ prefix ?

It was there before and I think it feels more descriptive, as the fields represent information about the texture.

> > * The number of textures *must* be equal to the number of planes of the video
> > format set in texture_info.
> 
> Why ? Won't that prevent few platforms from using it ?

As far as I have seen, the number of textures equals the number of planes of the given format. At least eglglessink used to assert this and glimagesink only supports RGBA (and asserts that n_textures == 1), so I have no example of another use case. Do you know one?

> As far as I have seen, the number of textures equals the number of planes of
> the given format. At least eglglessink used to assert this and glimagesink only
> supports RGBA (and asserts that n_textures == 1), so I have no example of
> another use case. Do you know one?

I don't. I've asked because I've remember that in some test we notice a performance hit when doing YUV in 3 planes compare to doing the same YUV in 1 plane (was on Mali). But thinking of it, it might not be possible to represent YUV planes correctly without having separate textures.

(In reply to comment #33)
> > As far as I have seen, the number of textures equals the number of planes of
> > the given format. At least eglglessink used to assert this and glimagesink only
> > supports RGBA (and asserts that n_textures == 1), so I have no example of
> > another use case. Do you know one?
> 
> I don't. I've asked because I've remember that in some test we notice a
> performance hit when doing YUV in 3 planes compare to doing the same YUV in 1
> plane (was on Mali). But thinking of it, it might not be possible to represent
> YUV planes correctly without having separate textures.

Ok, I think that if there ever is any need for doing all planes in one texture, then we could have a field in the "features" structure indicating this anomaly. Even if we keep the n_textures field, it is impossible for a sink to somehow infer the semantics of each different combination of textures number and planes number.

This is why I like the idea of introducing the "features" structure, because we can guarantee future compatibility with all kinds of weird devices without breaking our API again. I can understand that maybe more research is needed to find and document other hw anomalies, but I think it's a good idea to have it even if there is no other weird hw out there atm, just to ensure that we won't have to break our API if something comes up in the future.

(In reply to comment #34)
> Ok, I think that if there ever is any need for doing all planes in one texture,
> then we could have a field in the "features" structure indicating this anomaly.
> Even if we keep the n_textures field, it is impossible for a sink to somehow
> infer the semantics of each different combination of textures number and planes
> number.
> 
> This is why I like the idea of introducing the "features" structure, because we
> can guarantee future compatibility with all kinds of weird devices without
> breaking our API again. I can understand that maybe more research is needed to
> find and document other hw anomalies, but I think it's a good idea to have it
> even if there is no other weird hw out there atm, just to ensure that we won't
> have to break our API if something comes up in the future.

I just did my quick research, as I was suspicious about VDPAU, but indeed it's uses multiple textures for YV12 (NV12).

https://www.opengl.org/registry/specs/NV/vdpau_interop.txt

Looks like plan now.

What the status here ?

I don't really believe this is useful with the deprecation in bug 779965.