GNOME Bugzilla – Bug 734992
Constant BitRate mode encoding produces crappy video
Last modified: 2014-11-27 10:14:23 UTC
While enabling the CBR mode for encoding, the compressed video stream seems to be crappy during playback. sample pipelines: --------------------- gst-launch-1.0 videotestsrc num-buffers=500 ! vaapiencode_h264 rate-control=2 ! vaapiparse_h264 ! qtmux ! filesink location=sample.mp4 resulting video has bitrate of 542kb/s gst-launch-1.0 videotestsrc num-buffers=500 ! vaapiencode_h264 rate-control=2 bitrate=1024 ! vaapiparse_h264 ! qtmux ! filesink location=sample.mp4 resulting video has bitrate of 1Mb/s
The CBR mode is not at all working with gstreaer-vaapi-0.5.9 , but it is working fine with 0.5.8 . The reason is that we are submitting packed slice headers in 0.5.9 , but in 0.5.8 the driver itself is inserting the packed slice headers. This is affecting the slice_qp_delta in the the encoded slice_header. Right now the driver is finding out the QP dynamically after each frame encode and filling the slice_qp_delta (if the driver creates the packed_slice_header). In order to support this in gstreamer-vaapi (with packed_slice_header) we need to implement the whole rate control logic. Or do we have some other ways to do this?
Created attachment 284510 [details] [review] encoder: h264: Fix the period between I/P frames If the key-frame period is set as one, then ip_period shuld be zero. This patch is not addressing the core issue. But needed anyways.
The core issue was in the driver which is fixed here https://bugs.freedesktop.org/show_bug.cgi?id=83143 Without having the fix in #83143 , 0.5.9 release won't work with CBR mode unfortunately :(
(In reply to comment #3) > The core issue was in the driver which is fixed here > https://bugs.freedesktop.org/show_bug.cgi?id=83143 > > Without having the fix in #83143 , 0.5.9 release won't work with CBR mode > unfortunately :( Hmm, it seems that the "See Also" field doesn't work as I'd expect. :) I thought it would create a doubly-linked case.
(In reply to comment #4) > (In reply to comment #3) > > The core issue was in the driver which is fixed here > > https://bugs.freedesktop.org/show_bug.cgi?id=83143 > > > > Without having the fix in #83143 , 0.5.9 release won't work with CBR mode > > unfortunately :( > > Hmm, it seems that the "See Also" field doesn't work as I'd expect. :) > I thought it would create a doubly-linked case. But I can see corresponding links in both bugs now. Did you write them manually in both "See Also"?
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > The core issue was in the driver which is fixed here > > > https://bugs.freedesktop.org/show_bug.cgi?id=83143 > > > > > > Without having the fix in #83143 , 0.5.9 release won't work with CBR mode > > > unfortunately :( > > > > Hmm, it seems that the "See Also" field doesn't work as I'd expect. :) > > I thought it would create a doubly-linked case. > > > But I can see corresponding links in both bugs now. Did you write them manually > in both "See Also"? Manually added on both ends. :) At least, this is indeed much more convenient to find out, instead of digging through various comments.
> > > > > > Hmm, it seems that the "See Also" field doesn't work as I'd expect. :) > > > I thought it would create a doubly-linked case. > > > > > > But I can see corresponding links in both bugs now. Did you write them manually > > in both "See Also"? > > Manually added on both ends. :) > At least, this is indeed much more convenient to find out, instead of digging > through various comments. Yup , that make sense. Providing link itself instead of bug-id in whiteboard is more handy.
Review of attachment 284510 [details] [review]: This also assumes intra_period is always non-zero. ::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c @@ +1635,3 @@ seq_param->intra_period = GST_VAAPI_ENCODER_KEYFRAME_PERIOD (encoder); + seq_param->ip_period = (seq_param->intra_period == 1 ? 0 : 1) + + encoder->num_bframes; Maybe could we just have: seq_param->ip_period = seq_param->intra_period > 1 ? (1 + encoder->num_bframes) : 0; ? or seq_param->ip_period = (seq_param->intra_period > 1) + encoder->num_bframes; if we are sure about num_bframes range beforehand. BTW, related to this, I was also thinking about whether we shouldn't harden the check for num_bframes range too, but I believe this is already done in another function. Though, probably incorrectly. i.e. meanwhile, I would favor the first approach.
(In reply to comment #8) > Review of attachment 284510 [details] [review]: > > This also assumes intra_period is always non-zero. > > ::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c > @@ +1635,3 @@ > seq_param->intra_period = GST_VAAPI_ENCODER_KEYFRAME_PERIOD (encoder); > + seq_param->ip_period = (seq_param->intra_period == 1 ? 0 : 1) + > + encoder->num_bframes; > > Maybe could we just have: > seq_param->ip_period = seq_param->intra_period > 1 ? (1 + encoder->num_bframes) > : 0; ? this is giving more clarity :) ,, will change and push > > or > seq_param->ip_period = (seq_param->intra_period > 1) + encoder->num_bframes; if > we are sure about num_bframes range beforehand. > > BTW, related to this, I was also thinking about whether we shouldn't harden the > check for num_bframes range too, but I believe this is already done in another > function. Though, probably incorrectly. > We put some restriction to num_bframes by not allowing it to be more than (keyframe_period+1)/2. > i.e. meanwhile, I would favor the first approach.
encoder: h264: Fix the period between I/P frames If the key-frame period is set as one, then ip_period shuld be zero https://bugzilla.gnome.org/show_bug.cgi?id=734992
(In reply to comment #9) > (In reply to comment #8) > > Review of attachment 284510 [details] [review] [details]: > > > > This also assumes intra_period is always non-zero. > > > > ::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c > > @@ +1635,3 @@ > > seq_param->intra_period = GST_VAAPI_ENCODER_KEYFRAME_PERIOD (encoder); > > + seq_param->ip_period = (seq_param->intra_period == 1 ? 0 : 1) + > > + encoder->num_bframes; > > > > Maybe could we just have: > > seq_param->ip_period = seq_param->intra_period > 1 ? (1 + encoder->num_bframes) > > : 0; ? > > this is giving more clarity :) ,, will change and push > > > > > or > > seq_param->ip_period = (seq_param->intra_period > 1) + encoder->num_bframes; if > > we are sure about num_bframes range beforehand. > > > > BTW, related to this, I was also thinking about whether we shouldn't harden the > > check for num_bframes range too, but I believe this is already done in another > > function. Though, probably incorrectly. > > > > We put some restriction to num_bframes by not allowing it to be more than > (keyframe_period+1)/2. Yes, but if keyframe_period is 1, there is still a possibility that num_bframes is (user) set to 1. That was my point but anyway, the first construct covers this case now. :) > > > i.e. meanwhile, I would favor the first approach.