Looks to me like there's a bug somewhere else downstream, maybe in the audiosink?

If the gst_pad_alloc_buffer() returned GST_FLOW_OK the buffer should be of the size requested, I think. And in the case that such a check would actually be necessary, we would probably want to check for the right size then rather than just for >0.

I would also have expected such gst_pad_alloc_buffer() behaviour, but recently came across bug #635461 where it appears this need not always be so (in some not very clear unusual circumstances).

The buffer returned by gst_pad_alloc_buffer() has to be checked by the caller. It could be of different size or with different caps than requested and the caller must handle this. This is not very convient but that's how it is in 0.10

Different caps or sizes can happen for example if a capsfilter changes the caps

And what about the _and_set_caps variant ?  AFAICS, core does some additional stuff like configuring the src caps which would probably not allow for changing caps for an element with fixed src pad caps.

Though that still seems to leave the case where new (but equal) caps are provided with a smaller size.

The only difference of the _set_caps() variant is, that it automatically calls your srcpad's setcaps function. core doesn't do anything special for the non-set_caps() variant other than giving you the buffer provided by downstream.


Also downstream is not required to check if the different caps it returns (and in the case of capsfilter the size will be 0 btw, capsfilter can't calculate the size of buffers from the caps) are actually supported by upstream. Upstream always has to check the caps and size.

(In reply to comment #5)
> The only difference of the _set_caps() variant is, that it automatically calls
> your srcpad's setcaps function. core doesn't do anything special for the
> non-set_caps() variant other than giving you the buffer provided by downstream.
> 

... but only after calling gst_pad_accept_caps(), which should not succeed in a typical fixed src caps situation (for really different caps at least).

> 
> Also downstream is not required to check if the different caps it returns (and
> in the case of capsfilter the size will be 0 btw, capsfilter can't calculate
> the size of buffers from the caps) are actually supported by upstream. Upstream
> always has to check the caps and size.

Yes, upstream should check, but that could be core code, as in:

(from gst_pad_alloc_buffer_full())
  /* sanity check (only if caps are the same) */
  if (G_LIKELY (newcaps == caps) && G_UNLIKELY (GST_BUFFER_SIZE (*buf) < size))
    goto wrong_size_fallback;

In another world, it could/might always check regardless of whether caps are the same.

(In reply to comment #6)
> (In reply to comment #5)
> > The only difference of the _set_caps() variant is, that it automatically calls
> > your srcpad's setcaps function. core doesn't do anything special for the
> > non-set_caps() variant other than giving you the buffer provided by downstream.
> > 
> 
> ... but only after calling gst_pad_accept_caps(), which should not succeed in a
> typical fixed src caps situation (for really different caps at least).

Yes, if the _set_caps() variant is used that's true. If it isn't the caller has to do this.

> > 
> > Also downstream is not required to check if the different caps it returns (and
> > in the case of capsfilter the size will be 0 btw, capsfilter can't calculate
> > the size of buffers from the caps) are actually supported by upstream. Upstream
> > always has to check the caps and size.
> 
> Yes, upstream should check, but that could be core code, as in:
> 
> (from gst_pad_alloc_buffer_full())
>   /* sanity check (only if caps are the same) */
>   if (G_LIKELY (newcaps == caps) && G_UNLIKELY (GST_BUFFER_SIZE (*buf) < size))
>     goto wrong_size_fallback;
> 
> In another world, it could/might always check regardless of whether caps are
> the same.

Agreed, fortunately this is gone in 0.11 and replaced by something better ;)

Comment on attachment 203240 [details] [review]
patch to avoid segfault when filling in gst buffer

Partially committed (the state check)

commit 220b88fcc1ce0e203cc042a4dffef381a87bda1c
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Tue Dec 13 14:54:18 2011 +0100

    a52dec: Don't claim to support upstream renegotiation
    
    and use fixed caps on the srcpad. To correctly support
    upstream renegotiation a52dec would need to check if the
    caps of the downstream allocated buffer are the requested
    caps or if the size is different.
    
    Fixes bug #665989.

commit cdf8d0f6b2d784cf7cce360a310cef3adebc3d66
Author: Julien Isorce <julien.isorce@gmail.com>
Date:   Tue Dec 13 14:52:26 2011 +0100

    a52dec: Check that the a52_state is correctly initialized

This is not entirely fixed, see bug #666174

I cannot reproduce the problem since bug #666174 has been fixed (both before and after been ported to GstAudioEncoder)