GNOME Bugzilla – Bug 764421
libs: (audio,video) split allocation query caps and pad caps
Last modified: 2016-10-31 14:16:45 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
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.
Shall this be used in videoencoder too?
Yes, and for consistency the audio ones too probably?
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
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.
Created attachment 325144 [details] [review] libs: audio: split allocation query caps and pad caps
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.
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.
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.
(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 ();
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.
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.
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
Attachment 325353 [details] pushed as 37c4915 - libs: audio: split allocation query caps and pad caps