Don't we get any indication from libvpx when a frame is being dropped?

Here's another question: aiui a VPx encoder may actually output more frames than there were input frames (e.g. golden frames) - do we handle that case?

(In reply to Sebastian Dröge (slomo) from comment #1)
> Don't we get any indication from libvpx when a frame is being dropped?

No. Or at least, I couldn't find it.

(In reply to Tim-Philipp Müller from comment #2)
> Here's another question: aiui a VPx encoder may actually output more frames
> than there were input frames (e.g. golden frames) - do we handle that case?

No. There must be an equal number of frames on the input and output at the moment.

(In reply to George Kiagiadakis from comment #3)

> (In reply to Tim-Philipp Müller from comment #2)
> > Here's another question: aiui a VPx encoder may actually output more frames
> > than there were input frames (e.g. golden frames) - do we handle that case?
> 
> No. There must be an equal number of frames on the input and output at the
> moment.

That would be a regression then, maybe from moving to the vpxenc base class. I implemented that a long time ago

(In reply to Sebastian Dröge (slomo) from comment #4)
> (In reply to George Kiagiadakis from comment #3)
> > No. There must be an equal number of frames on the input and output at the
> > moment.
> 
> That would be a regression then, maybe from moving to the vpxenc base class.
> I implemented that a long time ago

Maybe I misunderstand something, but I fail to see how this would have worked in the past. GstVideoEncoder allocates a GstVideoCodecFrame internally in its chain() function. It then passes it down to the handle_frame() vfunc, while at the same time it stores it in an internal queue (priv->frames). GstVpxEnc afterwards in handle_frame() uses that GstVideoCodecFrame to call the encode function and then unrefs it (but without removing it from the queue). Later, in the gst_vpx_enc_processs() function, GstVideoCodecFrames are retrieved from the queue, filled with an output buffer and sent to the finish_frame() function, which properly releases the GstVideoCodecFrame and removes it from the queue as well.

Now, aiui, this is a 1-to-1 match. There is no way for GstVpxEnc (or any other encoder) to create a new GstVideoCodecFrame, since gst_video_encoder_new_frame() is not public API, therefore if you get more frames on the output than how many you have on the input, you are going to run out of GstVideoCodecFrames.

I don't fully understand why GstVideoEncoder is designed this way, but it looks like it would have been simpler to separate the input from the output frame objects and just release the input as soon as it is not needed, while at the same time create new objects on demand at the output. This would have avoided this bug, and possibly others.

Theoretically the subclass could ref the GstVideoCodecFrame when it sees a golden frame, and call _finish() once with the first output buffer, and then (knowing that it has been pushed) set the output buffer in the codec frame to the next frame and call _finish() again. If that works in practice I don't know :)

You can just directly push a buffer downstream if there's no corresponding GstVideoCodecFrame.

(In reply to Sebastian Dröge (slomo) from comment #7)
> You can just directly push a buffer downstream if there's no corresponding
> GstVideoCodecFrame.

Golden frames are still supported. They are passed to the subclass using VpxEnc::handle_invisible_frame_buffer and push using pad_push using VideoEnc::pre_push.

Review of attachment 352566 [details] [review]:

::: ext/vpx/gstvpxenc.c
@@ +1697,3 @@
+        frame = gst_video_encoder_get_oldest_frame (video_encoder);
+      } while (expected_frame_number != 0 &&
+          frame->system_frame_number != expected_frame_number);

Interestingly, I have strong doubt that STATS packets replaces an encoded frame. I'm starting to think that this code is totally wrong, that this block should simply be removed.

@@ +1724,3 @@
+      frame = gst_video_encoder_get_oldest_frame (video_encoder);
+    } while (expected_frame_number != 0 &&
+        frame->system_frame_number != expected_frame_number);

I have my doubts about the correctness of using the frame number here. What VPX seems to carry, is temporal information about the frames. It's unfortunate we give the user control over the resolution, but yet, I think it still usable for sorting out which frames are to be removed.

If you look attentively, the pkt contains a timestamp which is carried from the input.

(In reply to Nicolas Dufresne (stormer) from comment #9)
> Review of attachment 352566 [details] [review] [review]:
> 
> ::: ext/vpx/gstvpxenc.c
> @@ +1697,3 @@
> +        frame = gst_video_encoder_get_oldest_frame (video_encoder);
> +      } while (expected_frame_number != 0 &&
> +          frame->system_frame_number != expected_frame_number);
> 
> Interestingly, I have strong doubt that STATS packets replaces an encoded
> frame. I'm starting to think that this code is totally wrong, that this
> block should simply be removed.

Ignore this one, in first pass mode, only stats packets are produced. The fake buffer is just to make the flow working and to prevent leaks that same way you do now.

Ping, any feedback about using the frame number vs using timestamp ?

(In reply to Nicolas Dufresne (stormer) from comment #11)
> Ping, any feedback about using the frame number vs using timestamp ?

I guess it will work, assuming the encoder doesn't modify timestamps...
I wonder what the timestamp of the STATS packet will be, though. Note that the stats packet uses the 'twopass_stats' member of the packet union instead of the 'frame' one, which has the timestamp. It's all still a bit fuzzy.

Created attachment 356478 [details] [review]
vpxenc: discard frames that have been dropped by libvpx

I've tried using the pts as an experiment. It seems to work perfectly and solves the issue as well. I didn't do this for the STATS packets, as I'm not sure how they should be handled, but I'm thinking that maybe in their case we don't have to do anything. The STATS packet will be matched to an old GstVideoCodecFrame and then we will catch up when a normal packet arrives.

Review of attachment 356478 [details] [review]:

Looks good to me, it's also simplier now.

commit 36fc2a747ade50db69221a502d498efa890bf054 (HEAD -> master, origin/master, origin/HEAD)
Author: George Kiagiadakis <george.kiagiadakis@collabora.com>
Date:   Thu Jul 27 17:21:48 2017 +0300

    vpxenc: discard frames that have been dropped by libvpx
    
    This fixes a memory leak. When dropframe-threshold has been set,
    libvpx may output less frames than the input ones, which causes
    some GstVideoCodecFrames to queue up in GstVideoEncoder's internal
    frame queue with no chance of ever being all released. And because
    the frames keep references to the input buffers, the input buffer
    pool keeps allocating new buffers and memory usage grows very fast.
    For example the following pipeline's memory usage grows at a rate
    of about 1GB per minute!
    
    videotestsrc ! capsfilter caps=video/x-raw,width=1920,height=1080,framerate=30/1,format=I420 ! \
      vp8enc target-bitrate=1000000 end-usage=cbr dropframe-threshold=95 ! fakesink
    
    https://bugzilla.gnome.org/show_bug.cgi?id=783086

Also for 1.12?

(In reply to Sebastian Dröge (slomo) from comment #16)
> Also for 1.12?

Make sense.