I agree, this is something we usually fully do in the baseclass, anyone see a problem with change ? I *think* it should not react badly with decoders that do unset the flag themeself, but better check then being sorry.

Yes, see my comment in the other bug :)

Review of attachment 263299 [details] [review]:

Fix the commit message please, just start with videodecoder: and add the bug URL

::: gst-libs/gst/video/gstvideodecoder.c
@@ +3198,2 @@
   GST_VIDEO_DECODER_STREAM_LOCK (decoder);
+  if (klass->negotiate && gst_pad_check_reconfigure (decoder->srcpad)) {

This means that calling negotiate explicitly might not trigger another try of negotiation. This is a change in behaviour, and e.g. will cause problems if priv->output_state_changed but the reconfigure flag is not set on the pad.

Always call ->negotiate(), and then mark the pad is not needing reconfiguration if negotiation succeeded

I think if the  downstream element sends a reconfigure event, the call to "klass->negotiate (decoder)" from "gst_video_decoder_negotiate" will be done automatically when the decoder calls any of the following functions:

gst_video_decoder_allocate_output_buffer
gst_video_decoder_allocate_output_frame
gst_video_decoder_finish_frame


But in case this is not valid, here are what you asked:

  if (klass->negotiate)
    ret = klass->negotiate (decoder);

  /* clear GST_PAD_FLAG_NEED_RECONFIGURE if negotiation succeeded */
  if (ret)
    gst_pad_check_reconfigure (decoder->srcpad);

Is it what you thought ?

(In reply to comment #4)
> I think if the  downstream element sends a reconfigure event, the call to
> "klass->negotiate (decoder)" from "gst_video_decoder_negotiate" will be done
> automatically when the decoder calls any of the following functions:
> 
> gst_video_decoder_allocate_output_buffer
> gst_video_decoder_allocate_output_frame
> gst_video_decoder_finish_frame

Yes, it will be called in these cases. But also subclasses often call it from e.g. GstVideoDecoder::set_format() to let negotiation happen ASAP.

> But in case this is not valid, here are what you asked:
> 
>   if (klass->negotiate)
>     ret = klass->negotiate (decoder);
> 
>   /* clear GST_PAD_FLAG_NEED_RECONFIGURE if negotiation succeeded */
>   if (ret)
>     gst_pad_check_reconfigure (decoder->srcpad);
> 
> Is it what you thought ?

Well, it's racy :) But in principle yes. I think to make it not racy you'll have to call check_reconfigure() first to unmark it in any case... and if negotiate() fails you mark it for reconfiguration again. (or probably just mark it always for reconfiguration if it fails... if it fails things have to be retried ASAP or fail completely).

Created attachment 263387 [details] [review]
gstvideodecoder: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if gst_video_decoder_negotiate succeeds

Add gst_video_decoder_negotiate_unlocked for internal uses. So that we can have more control of what we are doing inside GstVideoDecoder.

And now gst_video_decoder_negotiate always clear the reconfigure flag. And set it again if negotiate fails.

(In reply to comment #5)
> (In reply to comment #4)
> > I think if the  downstream element sends a reconfigure event, the call to
> > "klass->negotiate (decoder)" from "gst_video_decoder_negotiate" will be done
> > automatically when the decoder calls any of the following functions:
> > 
> > gst_video_decoder_allocate_output_buffer
> > gst_video_decoder_allocate_output_frame
> > gst_video_decoder_finish_frame
> 
> Yes, it will be called in these cases. But also subclasses often call it from
> e.g. GstVideoDecoder::set_format() to let negotiation happen ASAP.

I think 

need_reconfigure = check_reconfigure (pad);
"if (klass->negociate && need_reconfigure) then negotiate() ..."

would be ok also for the asap case GstVideoDecoder::set_format() because a pad is initially marked GST_PAD_FLAG_NEED_RECONFIGURE (see comment #1)

But it depends if a pad goes to inactive when going to ready state. And I'm not sure about that.

Review of attachment 263387 [details] [review]:

::: gst-libs/gst/video/gstvideodecoder.c
@@ +3182,3 @@
 
+static gboolean
+gst_video_decoder_negotiate_unlocked (GstVideoDecoder * decoder)

Why do you add this? Note that the stream lock is a recursive lock

It was mainly to avoid double check of the flag, In the previous patch, there were gst_pad_needs_reconfigure and then gst_pad_check_reconfigure.
It also allows to have more control on what we choose to do inside GstVideoDecoder so that we do not depends on the gst_video_decoder_negotiate behavior that can be called from outside.

Then I used this opportunity to also remove the lock even if recursive. Maybe the function should be renamed to gst_video_decoder_negotiate_unchecked or something.

Let me know what do you think (and also about comment #7) and I will remake the whole patch :)

Comment on attachment 263387 [details] [review]
gstvideodecoder: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if gst_video_decoder_negotiate succeeds

No, I'm ok with that. Please do the same change in audiodecoder, audioencoder and videodecoder

Created attachment 263495 [details] [review]
videoencoder: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if negotiate succeeds

Created attachment 263496 [details] [review]
audiodecoder/enc: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if negotiate succeeds

Review of attachment 263387 [details] [review]:

::: gst-libs/gst/video/gstvideodecoder.c
@@ +3249,3 @@
   if (G_UNLIKELY (!decoder->priv->output_state
           || decoder->priv->output_state_changed
           || gst_pad_check_reconfigure (decoder->srcpad))) {

There's a bug here btw, and in all the other patches. You only check and unset the reconfigure flag if any of the other conditions fails. So will go here twice still if first one of the other conditions is true.

Comment on attachment 263495 [details] [review]
videoencoder: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if negotiate succeeds

Looks good except for the same problem as in videodecoder

Comment on attachment 263496 [details] [review]
audiodecoder/enc: clear GST_PAD_FLAG_NEED_RECONFIGURE flag if negotiate succeeds

Looks good except for the same problem as in videodecoder

ah right, nice catch :) thx.

So do you think I should go back to the first solution (using gst_pad_needs_reconfigure and using the same negotiate function) ? or move gst_pad_check_reconfigure (decoder->srcpad) to the first place in the if ?

I would put it to the first place in the if, or just call it unconditionally before the if and remember the return value.

Created attachment 263592 [details] [review]
videodec/enc: clear reconfigure flag if negotiate succeeds

Created attachment 263593 [details] [review]
audiodec/enc: clear reconfigure flag if negotiate succeeds

commit e68317f070caf533c582c9fe2795e21b75f4747f
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Thu Dec 5 14:39:57 2013 +0000

    audiodec/enc: clear reconfigure flag if negotiate succeeds
    
    So that it avoids to send an allocation query twice.
    One from an early call to gst_audio_encoder_negotiate from a
    subclass, then one from gst_audio_encoder_allocate_output_buffer.
    Which means that previously gst_audio_encoder_negotiate was not
    clearing the GST_PAD_FLAG_NEED_RECONFIGURE even on success.
    
    Fixes bug https://bugzilla.gnome.org/show_bug.cgi?id=719684

commit 79ef75888c88c5883b8963c9e85d4260fd57f72d
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Thu Dec 5 14:31:25 2013 +0000

    videodec/enc: clear reconfigure flag if negotiate succeeds
    
    So that it avoids to send an allocation query twice.
    One from an early call to gst_video_encoder_negotiate from a
    subclass, then one from gst_video_encoder_allocate_output_frame.
    Which means that previously gst_video_encoder_negotiate was not
    clearing the GST_PAD_FLAG_NEED_RECONFIGURE even on success.
    
    Fixes bug https://bugzilla.gnome.org/show_bug.cgi?id=719684

Backported to the 1.2 branch. Changing the target milestone accordingly.