GNOME Bugzilla – Bug 786320
vaapih264enc: disable periodic keyframe
Last modified: 2018-02-19 21:39:22 UTC
Created attachment 357627 [details] [review] patch to increase max period_keyframe Current implementation of vaapih264enc enforces a keyframe each 300 frames at maxinum. This is translated to an IDR each 10 seconds at 30 fps or 5 seconds at 60 fps. An IDR frame degrade the quality in the case you have a constrained bandwidth (you can see blocks each 5 or 10 seconds). In order to solve this scenario, I increased the max value of periodic_keyframe to a higher value (MAX_UINT). THe keyframe is generated only when there is a force_keyframe event inside the encoder pipeline. I tested with H.264 and I didn't see an issue or artifact appear after awhile. There is another reason to kept the max value limited to 300? I attaching my changes for testing.
Comment on attachment 357627 [details] [review] patch to increase max period_keyframe This is just, as the previous comment states, a testing patch, thus marking it accordingly.
The issue here is how to solve it and in which cases we should solve this issue. Since there are a ton of other use-cases.
Hi, There is an evolution of this report? I would like to add that x264enc plugin uses the same approach provided in the patch. I would like also point that we don't need to change the default value so let the current behavior in force.
Review of attachment 357627 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiencoder.c @@ +165,3 @@ g_param_spec_uint ("keyframe-period", "Keyframe Period", + "Maximal distance between two keyframes (0: auto-calculate)", 1, 4294967295, Also change this number to G_MAXINT ::: gst-libs/gst/vaapi/gstvaapiutils_h26x_priv.h @@ +32,2 @@ /* Define the maximum IDR period */ +#define MAX_IDR_PERIOD 4294967295 Please change to G_MAXINT if that's the value you want, as if ffmpeg
Thanks, I'll do that.
Created attachment 365492 [details] [review] Patch for extend max value for periodic key frame. Review and updated patch for extending periodic key frame to a higher value.
I noted that in the vaapih264 encoder there is a couple of re-defines of MAX_IDR_PERIOD. I don't know why and if there is a reason for that, so, I changed the values across all occurrences of MAX_IDR_PERIOD instead of removing the re-define. But I think this multiple defines should be removed and include the header in the implementation file. (this could be part of another bug report)
Hi, Any comment on the latest patch? Thanks Matteo
Review of attachment 365492 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiencoder.c @@ +165,3 @@ g_param_spec_uint ("keyframe-period", "Keyframe Period", + "Maximal distance between two keyframes (0: auto-calculate)", 1, G_MAXINT, Reading the libva code, this value boils down to a uint32 variable (https://github.com/intel/libva/blob/master/va/va_enc_h264.h#L171 and https://github.com/intel/libva/blob/master/va/va_enc_hevc.h#L175) But G_MAXINT is platform specific, so rethinking it again, I would say it is better to set it to G_MAXINT32 Another bug there is that we are saying that 0 is auto-calculate but we are not allowing to set zero, since 1 is the minimal value permitted. ::: gst-libs/gst/vaapi/gstvaapiutils_h26x_priv.h @@ +32,2 @@ /* Define the maximum IDR period */ +#define MAX_IDR_PERIOD G_MAXINT This is as sensible option, but I don't feel comfortable with it. Perhaps it is a better idea to remove that macro and replace its usage with G_MAXINT32, because we would be only hiding the fact that the max keyframe period is only bounded by the int32 max value, not a "magic number" as it was before (512).
(In reply to Matteo Valdina from comment #8) > I noted that in the vaapih264 encoder there is a couple of re-defines of > MAX_IDR_PERIOD. It is because the support of Flexible Encoding Infrastructure, an experimental feature in VAAPI. Though most probably you will not use it, I will appreciate your patch also update that code. We need to refactor all that code (bug #787118)
(In reply to Víctor Manuel Jáquez Leal from comment #10) > Review of attachment 365492 [details] [review] [review]: > > ::: gst-libs/gst/vaapi/gstvaapiencoder.c > @@ +165,3 @@ > g_param_spec_uint ("keyframe-period", > "Keyframe Period", > + "Maximal distance between two keyframes (0: auto-calculate)", 1, > G_MAXINT, > > Reading the libva code, this value boils down to a uint32 variable > (https://github.com/intel/libva/blob/master/va/va_enc_h264.h#L171 and > https://github.com/intel/libva/blob/master/va/va_enc_hevc.h#L175) > > But G_MAXINT is platform specific, so rethinking it again, I would say it is > better to set it to G_MAXINT32 G_MAXUINT32 > > Another bug there is that we are saying that 0 is auto-calculate but we are > not allowing to set zero, since 1 is the minimal value permitted. > > ::: gst-libs/gst/vaapi/gstvaapiutils_h26x_priv.h > @@ +32,2 @@ > /* Define the maximum IDR period */ > +#define MAX_IDR_PERIOD G_MAXINT > > This is as sensible option, but I don't feel comfortable with it. Perhaps it > is a better idea to remove that macro and replace its usage with G_MAXINT32, > because we would be only hiding the fact that the max keyframe period is > only bounded by the int32 max value, not a "magic number" as it was before > (512). G_MAXUINT32
Thanks for your comments. About the comment: " Another bug there is that we are saying that 0 is auto-calculate but we are not allowing to set zero since 1 is the minimal value permitted. " This is another separate pre-existing bug. I can easily update the comment from "Maximal distance between two keyframes (0: auto-calculate)" to "Maximal distance between two keyframes". I didn't see the code to auto calculate the keyframes distance, so, the zero case looks like is not implemented. Best Matteo
(In reply to Matteo Valdina from comment #13) > Thanks for your comments. > > About the comment: > " > Another bug there is that we are saying that 0 is auto-calculate but we are > not allowing to set zero since 1 is the minimal value permitted. > " > > This is another separate pre-existing bug. I can easily update the comment > from "Maximal distance between two keyframes (0: auto-calculate)" to > "Maximal distance between two keyframes". > > I didn't see the code to auto calculate the keyframes distance, so, the zero > case looks like is not implemented. Yes, indeed. But a default value is generated here https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/tree/gst-libs/gst/vaapi/gstvaapiencoder.c#n818 The code is a bit convoluted but it ends up as the default value of the idr_period (1 keyframe per second) if the idr_period is lower than that. But you are right, leave that for another bug. We shall not mix things.
Created attachment 368389 [details] [review] Patch for extend max value for periodic key frame (G_MAXUINT32). Hi, I update the original patch with G_MAXUINT32. I saw that the current code will perform something like: if (encoder->idr_period > G_MAXUINT32) encoder->idr_period = G_MAXUINT32; This if statement is not really needed because idr_period is a guint32 so it cannot be true. Anyway, I didn't change because idr_period could be changed in the future. If you want I can remove the unneeded check.
(In reply to Matteo Valdina from comment #15) > Created attachment 368389 [details] [review] [review] > Patch for extend max value for periodic key frame (G_MAXUINT32). > > Hi, > I update the original patch with G_MAXUINT32. > > I saw that the current code will perform something like: > > if (encoder->idr_period > G_MAXUINT32) > encoder->idr_period = G_MAXUINT32; > > This if statement is not really needed because idr_period is a guint32 so it > cannot be true. > Anyway, I didn't change because idr_period could be changed in the future. > > If you want I can remove the unneeded check. Yes, please remove those lines. Thanks.
Created attachment 368420 [details] [review] Patch for extend max value for periodic key frame (G_MAXUINT32 cleanup). Removed redundant check.
Attachment 368420 [details] pushed as 83886ced - libs: encoder: h264,h265: extend max periodic keyframe. I changed a bit the summary. Thanks!