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 764421 - libs: (audio,video) split allocation query caps and pad caps
libs: (audio,video) split allocation query caps and pad caps
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other All
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 764316
 
 
Reported: 2016-03-31 15:40 UTC by Víctor Manuel Jáquez Leal
Modified: 2016-10-31 14:16 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
libs: split allocation query caps and pad caps (2.46 KB, patch)
2016-03-31 15:40 UTC, Víctor Manuel Jáquez Leal
needs-work Details | Review
libs: video: split allocation query caps and pad caps (4.00 KB, patch)
2016-04-01 11:05 UTC, Víctor Manuel Jáquez Leal
none Details | Review
libs: audio: split allocation query caps and pad caps (4.72 KB, patch)
2016-04-01 11:05 UTC, Víctor Manuel Jáquez Leal
none Details | Review
libs: video: split allocation query caps and pad caps (4.02 KB, patch)
2016-04-01 11:08 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
libs: audio: split allocation query caps and pad caps (4.88 KB, patch)
2016-04-01 11:08 UTC, Víctor Manuel Jáquez Leal
none Details | Review
libs: audio: split allocation query caps and pad caps (6.72 KB, patch)
2016-04-04 14:55 UTC, Víctor Manuel Jáquez Leal
none Details | Review
libs: audio: split allocation query caps and pad caps (6.72 KB, patch)
2016-04-04 15:28 UTC, Víctor Manuel Jáquez Leal
committed Details | Review

Description Víctor Manuel Jáquez Leal 2016-03-31 15:40:08 UTC
According to [1], if I understand it correctly, we can have a situation where 
the allocations query caps may be different from the announced caps in the 
source pad. This situation is not handled by gstvideodecoder, it assumes that
the caps in the allocation query and the caps in the srcpad are the same.

The attached patch adds splits that caps logic adding a alloc_caps to 
GstVideoCodecState which, will be a copy of state's caps if it is not 
assigned.

1. https://bugzilla.gnome.org/show_bug.cgi?id=762543#c12
Comment 1 Víctor Manuel Jáquez Leal 2016-03-31 15:40:14 UTC
Created attachment 325094 [details] [review]
libs: split allocation query caps and pad caps

Allocation query caps contains memory size and the pad's caps contains the
display size.

This patch splits this logic distinction for videodecoder.
Comment 2 Víctor Manuel Jáquez Leal 2016-03-31 16:08:39 UTC
Shall this be used in videoencoder too?
Comment 3 Sebastian Dröge (slomo) 2016-04-01 06:52:27 UTC
Yes, and for consistency the audio ones too probably?
Comment 4 Sebastian Dröge (slomo) 2016-04-01 06:54:51 UTC
Review of attachment 325094 [details] [review]:

::: gst-libs/gst/video/gstvideodecoder.c
@@ +3810,3 @@
     state->caps = gst_video_info_to_caps (&state->info);
+  if (state->alloc_caps == NULL)
+    state->alloc_caps = gst_caps_ref (state->caps);

So the negotiate() implementation of the subclass would set this to something if needed? Maybe allocation_caps instead of omitting half a word?

::: gst-libs/gst/video/gstvideoutils.h
@@ +69,1 @@
   GstBuffer *codec_data;

This breaks ABI. You can't insert a new field somewhere in the middle
Comment 5 Víctor Manuel Jáquez Leal 2016-04-01 11:05:03 UTC
Created attachment 325143 [details] [review]
libs: video: split allocation query caps and pad caps

Allocation query caps contains memory size and the pad's caps contains the
display size.

This patch splits this logic distinction for videodecoder.
Comment 6 Víctor Manuel Jáquez Leal 2016-04-01 11:05:08 UTC
Created attachment 325144 [details] [review]
libs: audio: split allocation query caps and pad caps
Comment 7 Víctor Manuel Jáquez Leal 2016-04-01 11:08:03 UTC
Created attachment 325145 [details] [review]
libs: video: split allocation query caps and pad caps

Allocation query caps contains memory size and the pad's caps contains the
display size.

This patch splits this logic distinction for videodecoder and videoencoder.
Comment 8 Víctor Manuel Jáquez Leal 2016-04-01 11:08:08 UTC
Created attachment 325146 [details] [review]
libs: audio: split allocation query caps and pad caps

Allocation query caps contains memory size and the pad's caps contains the
display size.

This patch splits this logic distinction for audiodecoder and audioencoder.
Comment 9 Sebastian Dröge (slomo) 2016-04-02 08:00:15 UTC
Review of attachment 325146 [details] [review]:

::: gst-libs/gst/audio/audio-info.h
@@ +81,3 @@
   gint                      bpf;
   GstAudioChannelPosition   position[64];
+  GstCaps                  *allocation_caps;

This shouldn't be part of the GstAudioInfo. It has nothing to do with it and also breaks ABI (assumption is that GstAudioInfo does not have to be freed).

It has to be stored in something in the encoder/decoder base class, like the GstVideoCodecState in the video ones.
Comment 10 Víctor Manuel Jáquez Leal 2016-04-04 13:43:06 UTC
(In reply to Sebastian Dröge (slomo) from comment #9)
> Review of attachment 325146 [details] [review] [review]:
> 
> ::: gst-libs/gst/audio/audio-info.h
> @@ +81,3 @@
>    gint                      bpf;
>    GstAudioChannelPosition   position[64];
> +  GstCaps                  *allocation_caps;
> 
> This shouldn't be part of the GstAudioInfo. It has nothing to do with it and
> also breaks ABI (assumption is that GstAudioInfo does not have to be freed).
> 
> It has to be stored in something in the encoder/decoder base class, like the
> GstVideoCodecState in the video ones.

Yes, I'm a bit lost in audio. My approach now is to add allocate_caps in GstAudio{De,En}coderContext (part of element's private data structure) and add a method gst_audio_{en,de}coder_set_allocation_caps ();
Comment 11 Víctor Manuel Jáquez Leal 2016-04-04 14:55:02 UTC
Created attachment 325349 [details] [review]
libs: audio: split allocation query caps and pad caps

Allocation query caps contains memory size and the pad's caps contains the
display size.

This patch splits this logic distinction for audiodecoder and audioencoder.
Comment 12 Víctor Manuel Jáquez Leal 2016-04-04 15:28:32 UTC
Created attachment 325353 [details] [review]
libs: audio: split allocation query caps and pad caps

Allocation query caps contains memory size and the pad's caps contains the
display size.

This patch splits this logic distinction for audiodecoder and audioencoder.
Comment 13 Sebastian Dröge (slomo) 2016-04-05 05:47:55 UTC
Review of attachment 325353 [details] [review]:

Looks good except for:

::: gst-libs/gst/audio/gstaudiodecoder.c
@@ +3278,3 @@
+ * pad's caps. Use this function before calling
+ * gst_audio_decoder_negotiate(). Setting to %NULL the allocation
+ * query will use the caps from the pad.

Since: 1.10

::: gst-libs/gst/audio/gstaudioencoder.c
@@ +2397,3 @@
+ * pad's caps. Use this function before calling
+ * gst_audio_encoder_negotiate(). Setting to %NULL the allocation
+ * query will use the caps from the pad.

Since: 1.10
Comment 14 Víctor Manuel Jáquez Leal 2016-04-05 09:56:14 UTC
Attachment 325353 [details] pushed as 37c4915 - libs: audio: split allocation query caps and pad caps