I also tried with 1.2.4 
pipeline : appsrc ! videoconvert ! ximagesink
same error.

plus : 
if I changed the caps 
height+=10
to
height-=10

everything seems OK , is this a bug related to buffer pool reuse ?

If you would've copied what I've written on the mailing list, then the cause and solution would already be mentioned here :)



This looks like a bug indeed, I can't see anything wrong in your code.
The problem here most likely is that appsrc does not make sure that all
previous buffers are pushed downstream before using the new caps. Should
be relatively easy to fix by tracking caps together with buffers...

I'm on it. Like butter on bread! :)

(In reply to comment #2)
> If you would've copied what I've written on the mailing list, then the cause
> and solution would already be mentioned here :)
> 
> 
> 
> This looks like a bug indeed, I can't see anything wrong in your code.
> The problem here most likely is that appsrc does not make sure that all
> previous buffers are pushed downstream before using the new caps. Should
> be relatively easy to fix by tracking caps together with buffers...

any update ? after re-check git(73646bd0) version ,this bug still exists,
seems bug happend here: gstappsrc.c:1058
gst_app_src_create callback

  while (TRUE) {
    /* return data as long as we have some */
    if (!g_queue_is_empty (priv->queue)) {
      guint buf_size;

      if (priv->new_caps) {

         
        priv->new_caps = FALSE;

// did not flush previous buffers before new caps negotiate
    
        gst_app_src_do_negotiate (bsrc);
        /* Lock has released so now may need
         *- flushing
         *- new caps change
         *- check queue has data */
        if (G_UNLIKELY (priv->flushing))
          goto flushing;
        /* Contiue checks caps and queue */
        continue;--------> next loop may return a buffer but downstream elements already has new caps

how could I fix this ?

plus: I found gst_app_src_send_event drop all queued buffer when receiving flush_stop, does this mean appsrc lost buffers ?

Created attachment 281836 [details] [review]
misc patch to improve caps update logic

call gst_app_src_flush_queued in gst_app_src_set_caps should avoid giving out invalid size buffer, but if caps changing rapidly (consider new caps per buffer),lots buffer lost and maybe none buffer can gives out, maybe queue of buffer queue like :
queue: | newcaps,buf....|newcaps,buf....|newcaps,buf....
will be better?


while walking the code, I found gst_app_src_set_caps only compare caps pointer to determine if they are the same,should it use gst_caps_is_equal to compare as well ? if this is the case, this patch adding this.

You should not flush old buffers, but instead drain them first (so that downstream can handle all the old buffers with the old caps and no data gets lost)... and only then handle the new caps

Comment on attachment 281836 [details] [review]
misc patch to improve caps update logic

Why do you think this is needed? It shouldn't make a difference

I don't think this will work like this. You basically need to put the current caps next to the buffer when you queue them internally, so that caps changes and buffer pushes to the appsrc are serialized.

(In reply to comment #7)
> (From update of attachment 281836 [details] [review])
> Why do you think this is needed? It shouldn't make a difference

if different pointer have equal caps ,shouldn't this avoid one time negotiate ?

Created attachment 281900 [details] [review]
a draft patch to serialize caps set action together with buffer

this is a patch try to solve the problem ,it modified appsrc as following:

queue new caps after buffer in priv->queue (or replace tail caps)
add last_caps to store last caps setted to appsrc, to help check if new caps is changed
add current_caps to store current caps used , .(which is used in last gst_base_src_set_caps call)

for convenience, current/last_caps is nevel NULL, but use gst_caps_new_any

it leaves some problem, which caps should gst_app_src_get_caps return ? 
docs said "Get the configured caps on appsrc " in original implementation, it returns new caps even if haven't been used, is this correct when it's called as basesrc_class->get_caps ?  if caps is queued ,which caps should be returned ?

please feel free to correct this

Basically what you need to do here is to put the caps into the queue in order with the buffers. And not just replace pending caps, but apply them exactly where they are relative to the buffers.

Created attachment 281987 [details] [review]
v2 fix default negotiate

(In reply to comment #11)
> Basically what you need to do here is to put the caps into the queue in order
> with the buffers. And not just replace pending caps, but apply them exactly
> where they are relative to the buffers.

That's what I tried in the patch .it can fix the invalid size problem. but I have question about which caps should I return when basesrc_class->get_caps is called, should it be next pending caps, or current used caps ?

The currently used caps

Review of attachment 281987 [details] [review]:

::: gst-libs/gst/app/gstappsrc.c
@@ +526,3 @@
   priv->min_percent = DEFAULT_PROP_MIN_PERCENT;
+  priv->last_caps = gst_caps_new_any ();
+  priv->current_caps = gst_caps_new_any ();

Why distinguish current and last caps?

The caps property should always return the last caps, while the current caps are the currently used ones. That's the reason, right? Makes sense :)

And why not keep them at NULL and handle that as before?

@@ +541,3 @@
+      gst_buffer_unref (caps_or_buffer);
+    } else {
+      gst_caps_unref (caps_or_buffer);

You can use gst_mini_object_unref() on caps and buffers

@@ +1058,3 @@
     if (!g_queue_is_empty (priv->queue)) {
       guint buf_size;
+      void *caps_or_buffer;

Use GstMiniObject* as the type of these everywhere

@@ +1061,3 @@
+
+      caps_or_buffer = g_queue_peek_head (priv->queue);
+      if (GST_IS_CAPS (caps_or_buffer)) {

Why peek and later pop? Just integrating that loop directly into the while seems more useful

while (!empty) {
  obj = pop();
  if (caps) {
    negotiate();
  } else {
    handle buffer();
  }
}

Seems clearer

(In reply to comment #14)
> Review of attachment 281987 [details] [review]:
> 
> ::: gst-libs/gst/app/gstappsrc.c
> @@ +526,3 @@
>    priv->min_percent = DEFAULT_PROP_MIN_PERCENT;
> +  priv->last_caps = gst_caps_new_any ();
> +  priv->current_caps = gst_caps_new_any ();
> 
> Why distinguish current and last caps?
> 
> The caps property should always return the last caps, while the current caps
> are the currently used ones. That's the reason, right? Makes sense :)
there may be naming confusing in my patch, last_caps is the caps last app_src_set_caps setted, which is the tail caps in queue

so I should return current_caps in basesrc_class->get_caps
but return last_caps in app_src_get_caps. 
did I misunderstand something ? :)

last_caps also used to skip no-op caps set (set the same caps, or call app_src_set_caps many times but not pushing buffer bewteen them)

> 
> And why not keep them at NULL and handle that as before?
it makes caps equal test easier. but maybe not fit original style.
I'll try to correct this.

addition question: should gst_app_src_set_caps check if caps is fixed ? 
after queued , none-fixed caps will trigger gst_base_src_set_caps
->gst_pad_push_event->gst_event_new_caps failed, but not at the point wrong caps is setted. or should this be noticed in doc ?

Created attachment 282124 [details] [review]
v3 based on Sebastian Dröge (slomo) 's currection

based on Sebastian Dröge (slomo)'s currection, 
1.caps property  returns the last caps
2.use GstMiniObject* instead of void*
3.peek and later pop cleaned
4.last/current_caps use NULL value as before, my code seems too confused. hopes anyone gives better one.

Review of attachment 282124 [details] [review]:

::: gst-libs/gst/app/gstappsrc.c
@@ +537,3 @@
+  while ((caps_or_buffer = g_queue_pop_head (priv->queue))) {
+    if (caps_or_buffer) {
+      gst_mini_object_unref (caps_or_buffer);

Can it ever be NULL?

@@ +1088,2 @@
         /* Contiue checks caps and queue */
         continue;

Here I was more thinking about a symmetric loop instead of this

while (!g_queue_is_empty (priv->queue)) {
  obj = g_queue_pop_head (priv->queue);

  if (GST_IS_CAPS (obj)) {
    caps = GST_CAPS (obj);
    negotiate and stuff
  } else if (GST_IS_BUFFER (obj)) {
    buffer = GST_BUFFER (obj);
    push buffer downstream and everything
  }
  gst_mini_object_unref (obj);
}

@@ +1190,3 @@
   GST_DEBUG_OBJECT (appsrc, "setting caps to %" GST_PTR_FORMAT, caps);
+
+  caps_or_buffer = g_queue_peek_tail (priv->queue);

Why this complicated code? Just push the caps on the queue and handle them above :)

@@ +1251,3 @@
+  if ((caps = appsrc->priv->last_caps))
+    gst_caps_ref (caps);
+  GST_OBJECT_UNLOCK (appsrc);

Is the caps query now returning current_caps, while the caps property returns the last caps?

(In reply to comment #17)
> Review of attachment 282124 [details] [review]:
> 
> ::: gst-libs/gst/app/gstappsrc.c
> @@ +537,3 @@
> +  while ((caps_or_buffer = g_queue_pop_head (priv->queue))) {
> +    if (caps_or_buffer) {
> +      gst_mini_object_unref (caps_or_buffer);
> 
> Can it ever be NULL?

the original implementation allows set NULL as new caps ,so the queued caps can  also be NULL . that's also the reason I'm trying to use any caps instead of NULL caps:)

> 
> @@ +1088,2 @@
>          /* Contiue checks caps and queue */
>          continue;
> 
> Here I was more thinking about a symmetric loop instead of this
> 
> while (!g_queue_is_empty (priv->queue)) {
>   obj = g_queue_pop_head (priv->queue);
> 
>   if (GST_IS_CAPS (obj)) {
>     caps = GST_CAPS (obj);
>     negotiate and stuff
>   } else if (GST_IS_BUFFER (obj)) {
>     buffer = GST_BUFFER (obj);
>     push buffer downstream and everything
>   }
>   gst_mini_object_unref (obj);
> }
seems more clear. BTW,it may be a "NULL" as caps in queue, this should be considered

> 
> @@ +1190,3 @@
>    GST_DEBUG_OBJECT (appsrc, "setting caps to %" GST_PTR_FORMAT, caps);
> +
> +  caps_or_buffer = g_queue_peek_tail (priv->queue);
> 
> Why this complicated code? Just push the caps on the queue and handle them
> above :)

caller may set new caps multi times but didn't push any buffers between them, my patch tried to skip these unnecessary re-negotitate . if every new caps (even without buffer of this caps) event should be pushed downstream , I should correct my patch.
> 
> @@ +1251,3 @@
> +  if ((caps = appsrc->priv->last_caps))
> +    gst_caps_ref (caps);
> +  GST_OBJECT_UNLOCK (appsrc);
> 
> Is the caps query now returning current_caps, while the caps property returns
> the last caps?
yes, basesrc_class->get_caps points to gst_app_src_internal_get_caps and return  current_caps.  caps property use app_src_get_caps ,return last caps

(In reply to comment #18)
> (In reply to comment #17)
> > Review of attachment 282124 [details] [review] [details]:
> > 
> > ::: gst-libs/gst/app/gstappsrc.c
> > @@ +537,3 @@
> > +  while ((caps_or_buffer = g_queue_pop_head (priv->queue))) {
> > +    if (caps_or_buffer) {
> > +      gst_mini_object_unref (caps_or_buffer);
> > 
> > Can it ever be NULL?
> 
> the original implementation allows set NULL as new caps ,so the queued caps can
>  also be NULL . that's also the reason I'm trying to use any caps instead of
> NULL caps:)

Makes sense, keep it at NULL though. You can only set NULL caps on a pad, not ANY caps.

> > 
> > @@ +1190,3 @@
> >    GST_DEBUG_OBJECT (appsrc, "setting caps to %" GST_PTR_FORMAT, caps);
> > +
> > +  caps_or_buffer = g_queue_peek_tail (priv->queue);
> > 
> > Why this complicated code? Just push the caps on the queue and handle them
> > above :)
> 
> caller may set new caps multi times but didn't push any buffers between them,
> my patch tried to skip these unnecessary re-negotitate . if every new caps
> (even without buffer of this caps) event should be pushed downstream , I should
> correct my patch.

Good point. I think you would want to just put them always into the queue, but check if they are equal to the current caps right before setting them. That will keep the code a bit simpler while having the same effect.

Created attachment 282618 [details] [review]
v4 based on Sebastian Dröge (slomo) 's comment

(In reply to comment #19)
> Good point. I think you would want to just put them always into the queue, but
> check if they are equal to the current caps right before setting them. That
> will keep the code a bit simpler while having the same effect.
improved in new patch.

>Here I was more thinking about a symmetric loop instead of this
I didn't change to this, since there is continue/goto/nullable ,makes common gst_mini_object_unref difficult

commit 8fc307fae073631c4f6055b1bd2923aefec9e0f8
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Aug 6 10:07:42 2014 +0200

    appsrc: Some minor fixes and cleanup

commit 251c63c4abf7c2dfb10064fb17245392d000d546
Author: Wang Xin-yu (王昕宇) <comicfans44@gmail.com>
Date:   Wed Aug 6 09:59:32 2014 -0400

    appsrc: Make caps set action queued together with buffer
    
    https://bugzilla.gnome.org/show_bug.cgi?id=729760

Thanks Sebastian!