Hi, the leak probably happens elsewhere because the surface proxy is installed with gst_video_codec_frame_set_user_data() with an appropriate unref hook, which would be called on codec frame unref.

Indeed. The surface proxy is not leaked directly. We're leaking the GstVideoCodecFrame.

We get one reference when the frame is passed to gst_vaapidecode_handle_frame() and create another one in gst_vaapi_decoder_push_frame().

Usually the frame is handled in gst_vaapidecode_push_decoded_frame(). Here the frame is always released twice: gst_video_decoder_finish_frame() + gst_video_codec_frame_unref() or gst_video_decoder_drop_frame() + gst_video_codec_frame_unref().
In gst_vaapidecode_reset_full() each frame is only release once.

Calling gst_video_codec_frame_unref() twice (or gst_video_codec_frame_unref() + gst_video_decoder_drop_frame()?) in gst_vaapidecode_reset_full() should work, but I'm not sure if that's the correct fix. I don't understand the internals well enough to decide if we could drop the second reference elsewhere.

Created attachment 295510 [details] [review]
patch

How about this patch?

The gvd_handle_frame() will give the ownership of the frame to subclass which we release through gvd_finish_frame()/gvd_drop_frame().
The vaapidecoder is keeping an extra ref to the frames which are decoded and queued for output. This extra ref is released through the explicit gst_video_codec_frame_unref() just after gvd_finish_frame()/gvd_drop_frame().

The reset_full() routine get invoked for seek operation usually. In that case we only need to release the extra ref of queued frames. The other reference has been releasing by the base decoder(GstVideoDecoder) itself.

So i think the current code is correct, or do you have something else in your mind??

The base class will not release the ref passed to 'handle_frame'. It must be released in GstVaapiDecode. The documentation is quite clear about that:
"Each input frame is provided in turn to the subclass' handle_frame callback. The ownership of the frame is given to the handle_frame callback."

And check the code: gst_video_decoder_decode_frame() takes the ownership of the frame handed to it and passes it onto handle_frame(). It takes an extra ref for its own tracking that is released during reset.

(In reply to Michael Olbrich from comment #5)
> The base class will not release the ref passed to 'handle_frame'. It must be
> released in GstVaapiDecode. The documentation is quite clear about that:
> "Each input frame is provided in turn to the subclass' handle_frame
> callback. The ownership of the frame is given to the handle_frame callback."

Yes, did I say something against that ?:).. That ref has been released by invoking gvd_finish_frame() or gvd_drop_frame().. 

> And check the code: gst_video_decoder_decode_frame() takes the ownership of
> the frame handed to it and passes it onto handle_frame(). It takes an extra
> ref for its own tracking that is released during reset.

And where is this called for the frames released in gst_vaapidecode_reset_full()?

gst_vaapidecode_push_decoded_frame() calls _both_ gvd_finish_frame() or gvd_drop_frame() and gst_video_codec_frame_unref()
But gst_vaapidecode_reset_full() only calls gst_video_codec_frame_unref() I don't see where the other ref is released.

(In reply to Michael Olbrich from comment #7)
> And where is this called for the frames released in
> gst_vaapidecode_reset_full()?
> 
> gst_vaapidecode_push_decoded_frame() calls _both_ gvd_finish_frame() or
> gvd_drop_frame() and gst_video_codec_frame_unref()

This gst_video_codec_frame_unref() is to release the ref that we internally added (in gstvaapidecoder_objects.c)

> But gst_vaapidecode_reset_full() only calls gst_video_codec_frame_unref() I
> don't see where the other ref is released.

There is one frame_unref() in reset_full, which is to release the the explicit ref that we took internally (in gstvaapidecoder_objects.c). This was the only ref that we kept..The other ref(still owning by the basedecoder) has been removing by the basedecoder in gst_video_decoder_reset()...

(In reply to sreerenj from comment #8)
> There is one frame_unref() in reset_full, which is to release the the
> explicit ref that we took internally (in gstvaapidecoder_objects.c). This
> was the only ref that we kept..The other ref(still owning by the
> basedecoder) has been removing by the basedecoder in
> gst_video_decoder_reset()...

And this is where you're wrong. This is a different ref.
Check e.g. the path via gst_video_decoder_chain_forward():
- Here gst_video_decoder_decode_frame() is called with priv->current_frame and then priv->current_frame is set to NULL without releasing the frame.
-> that's one ref
- gst_video_decoder_decode_frame() takes another ref
-> that's two refs
- gst_vaapi_decoder_push_frame() takes another ref
-> that's three refs.

In the nomal path gst_video_decoder_finish_frame() releases _two_ refs via gst_video_decoder_release_frame(). The third is released via gst_video_codec_frame_unref()

With reset_full() only two of the refs are released: On in reset_full and one in gst_video_decoder_reset().
The patch should probably the changed to call gvd_drop_frame() instead but in this case it doesn't matter because gst_video_decoder_reset() clears all the internal state anyways.

(In reply to Michael Olbrich from comment #9)
> (In reply to sreerenj from comment #8)
> > There is one frame_unref() in reset_full, which is to release the the
> > explicit ref that we took internally (in gstvaapidecoder_objects.c). This
> > was the only ref that we kept..The other ref(still owning by the
> > basedecoder) has been removing by the basedecoder in
> > gst_video_decoder_reset()...
> 
> And this is where you're wrong. This is a different ref.
> Check e.g. the path via gst_video_decoder_chain_forward():
> - Here gst_video_decoder_decode_frame() is called with priv->current_frame
> and then priv->current_frame is set to NULL without releasing the frame.
> -> that's one ref
> - gst_video_decoder_decode_frame() takes another ref
> -> that's two refs
> - gst_vaapi_decoder_push_frame() takes another ref
> -> that's three refs.
> 
> In the nomal path gst_video_decoder_finish_frame() releases _two_ refs via
> gst_video_decoder_release_frame(). The third is released via
> gst_video_codec_frame_unref()
> 
> With reset_full() only two of the refs are released: On in reset_full and
> one in gst_video_decoder_reset().
> The patch should probably the changed to call gvd_drop_frame() instead but
> in this case it doesn't matter because gst_video_decoder_reset() clears all
> the internal state anyways.

Then it should need a fix in basedecoder, isn't it? Theoretically we only need to release the extra ref (if we have any) during reset/seek IMO...

Perhaps bug 745962 is related to this issue:

As you can see in attachment 299095 [details] [review], in the the gst_vaapidecode_flush() I had to add a gst_video_decoder_drop_frame (vdec, frame) before gst_video_codec_frame_unref (frame). This is because GstVideoDecoder holds a couple references of the frame until it's pushed or dropped.

I'm still surprised how it isn't required in gst_vaaapidecode_reset_full()

(In reply to Víctor Manuel Jáquez Leal from comment #11)
> Perhaps bug 745962 is related to this issue:
> 
> As you can see in attachment 299095 [details] [review] [review], in the the
> gst_vaapidecode_flush() I had to add a gst_video_decoder_drop_frame (vdec,
> frame) before gst_video_codec_frame_unref (frame). This is because
> GstVideoDecoder holds a couple references of the frame until it's pushed or
> dropped.
> 
> I'm still surprised how it isn't required in gst_vaaapidecode_reset_full()

Yes, I remember you have added it in that patch :)
Just wondering, it should have mentioned in upstream like "subclass need to invoke a gvd_drop_frame() while seeking",,, ??

(In reply to sreerenj from comment #12)
> (In reply to Víctor Manuel Jáquez Leal from comment #11)
> > Perhaps bug 745962 is related to this issue:
> > 
> > As you can see in attachment 299095 [details] [review] [review] [review], in the the
> > gst_vaapidecode_flush() I had to add a gst_video_decoder_drop_frame (vdec,
> > frame) before gst_video_codec_frame_unref (frame). This is because
> > GstVideoDecoder holds a couple references of the frame until it's pushed or
> > dropped.
> > 
> > I'm still surprised how it isn't required in gst_vaaapidecode_reset_full()
> 
> Yes, I remember you have added it in that patch :)
> Just wondering, it should have mentioned in upstream like "subclass need to
> invoke a gvd_drop_frame() while seeking",,, ??

It's required in both cases. The documentation is quite clear about that: The base class does not own the ref any more, so it cannot release it. The subclass must always release all refs owned by it.

Created attachment 301102 [details] [review]
updated patch

Basically the same as before:
- rebased to current master
-  use gst_video_decoder_drop_frame() instead of gst_video_codec_frame_unref()

(In reply to Michael Olbrich from comment #14)
> Created attachment 301102 [details] [review] [review]
> updated patch
> 
> Basically the same as before:
> - rebased to current master
> -  use gst_video_decoder_drop_frame() instead of
> gst_video_codec_frame_unref()

Your patch is not complete:

gstvaapidecode.c:646:36: error: use of undeclared identifier 'vdec'
      gst_video_decoder_drop_frame(vdec, out_frame);
                                   ^

You missed to include the vdec declaration.

I fixed the missing vdec declaration and the code style (there ought be an space before the open parenthesis). Then run a couple tests and it looks good.

Created attachment 301264 [details] [review]
vaapidecode: unref video codec frame twice

We get one reference when the frame is passed to decode_handle_frame()
and create another one in gst_vaapi_decoder_push_frame().

Usually the frame is handled in gst_vaapidecode_push_decoded_frame().
Here the frame is always released twice:
gst_video_decoder_finish_frame() + gst_video_codec_frame_unref() or
gst_video_decoder_drop_frame() + gst_video_codec_frame_unref().

In gst_vaapidecode_reset_full() both references to the frame must be
released as well.

Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>

commit a314b682b2520415a62c474611ea514308336fa5
Author: Michael Olbrich <m.olbrich@pengutronix.de>
Date:   Thu Dec 11 12:02:38 2014 +0100

    vaapidecode: unref video codec frame twice