Created attachment 170789 [details] [review]
x264enc: Use the new x264 API to request a keyframe

x264 now has an API to cleanly request a new keyframe, use that
instead of playing with random fields. This should work better
with intra-refresh mode.

This looks awfully messy, and it's not like the current force-keyframe stuff isn't a bit convoluted already - can we really not make this nicer?

Shouldn't it be enough to have exactly one single call to x264_encoder_intra_refresh() in the chain function?

Is it safe / a good idea to call x264_encoder_intra_refresh() from threads other than the streaming thread (src event handler in particular)?

We could call intra_refresh() only from the chain function, but then we'd have to keep a flag somewhere to remember to call it. Once we get rid of the code to support older versions of x264, it will be nicer.

It's safe because the encoder is protected by the object lock already (not the stream lock).

Actually, forget that, intra_refresh() should be used only when using periodic intra refresh, otherwise setting the frame type to IDR is appropriate.. updated patch coming.

Or x264_encoder_invalidate_reference()[1] can be used instead:
" 
*      When the client has packet loss or otherwise incorrectly decodes a frame, the encoder
*      can be told with this command to "forget" the frame and all frames that depend on it, referencing
*      only frames that occurred before the loss.  This will force a keyframe if no frames are left to
*      reference after the aforementioned "forgetting"."

[1]http://git.videolan.org/?p=x264.git;a=blob;f=x264.h;h=24c37926691f9298cb1d4b87f5e4a187d05347a0;hb=HEAD#l795

Created attachment 182888 [details] [review]
x264enc: Use API to force intra refresh if it is used

Much nicer patch on top of the patch from bug #644150

I also want to implement reference invalidating, but that needs more stuff in the event to klnow when the last valid frame was.

For RTP, we should be able to get it from the RPSI feedback message. My current idea was to extend the GstForceKeyUnit event, so that if the encoder doesn't do reference invalidation, it will degrade into a new keyframe.

(In reply to comment #7)
> I also want to implement reference invalidating, but that needs more stuff in
> the event to klnow when the last valid frame was.
> 
> For RTP, we should be able to get it from the RPSI feedback message. My current
> idea was to extend the GstForceKeyUnit event, so that if the encoder doesn't do
> reference invalidation, it will degrade into a new keyframe.

AFAIK RPSI packets are meant to use older reference pictures than the last one, but H264 is actually not mentioned in the specs, so it might behave differently. How would you force a key frame with RPSI?

RPSI doesnt force a keyframe and I'm not sure how to use it with H.264. But if the encoder doesn't support RPSI, it should fallback to having a new keyframe. That's why I wanted to use the same element for RPSI, slice keyframe request, full keyframe, etc.. So we can have fallback.

Review of attachment 182888 [details] [review]:

::: ext/x264/gstx264enc.c
@@ +1579,3 @@
+      x264_encoder_intra_refresh (encoder->x264enc);
+    else
+      pic_in.i_type = X264_TYPE_IDR;

pic_in.i_type is only set if FORCE_INTRA_API is definied but it should be set even if it's not defined:
#else
  pic_in.i_type = X264_TYPE_IDR;
#endif

Review of attachment 182888 [details] [review]:

::: ext/x264/gstx264enc.c
@@ +1579,3 @@
+      x264_encoder_intra_refresh (encoder->x264enc);
+    else
+      pic_in.i_type = X264_TYPE_IDR;

pic_in.i_type is only set if FORCE_INTRA_API is definied but it should be set even if it's not defined:
#else
  pic_in.i_type = X264_TYPE_IDR;
#endif

*** Bug 655611 has been marked as a duplicate of this bug. ***

This seems to be all upstream now.