Re: [Libva] [Libva-intel-driver][PATCH] Needn't reset brc if the bitrate setting isn't changed in the Begin/Render/End sequence
On Thu, 2016-12-29 at 18:52 +, Mark Thompson wrote:
> On 29/12/16 07:09, Zhao Yakui wrote:
> > On 12/29/2016 03:08 PM, Xiang, Haihao wrote:
> > >
> > > > On 12/29/2016 01:22 PM, Xiang, Haihao wrote:
> > > > > User can use VAEncMiscParameterRateControl to update bitrate,
> > > > > so we
> > > > > should ignore
> > > > > the bitrate in the sequence parameter if
> > > > > VAEncMiscParameterRateControl is present.
> > > >
> > > > This makes sense. The MiscRateControl mentions that it can
> > > > override
> > > > the
> > > > bps setting in sequence parameter.
> > > >
> > > > > Hence it is not needed to reset brc if
> > > > > VAEncMiscParameterRateControl doesn't change
> > > > > the used bitrate.
> > > >
> > > > Does it still need to send the VAEncMiscParameterRateControl
> > > > parameter
> > > > if the bps is not changed for the new sequence?
> > >
> > > Yes if bits_per_second in the sequence parameter is not equal to
> > > the
> > > value for the previous Begin/Render/End sequence except
> > > bits_per_second
> > > in the sequence parameter is 0.
> > >
> >
> > OK.
> > It makes sense.
> >
> > This looks good to me.
>
> I'm not sure this behaviour is correct. Should not the most recently
> given bitrate value from the user, either from sequence parameters or
> RC parameters, be used? When is_new_sequence is set, the user will
> have provided a set of sequence parameters (because we are making an
> IDR frame), so the bitrate there is provided by the user and should
> override any previous RC parameters given before that frame (but not
> any on this one).
>
> That is, I think it should work like:
>
> Sequence parameters with bps = 1M
> -> IDR frame, at 1Mbps
> -> some P frames, at 1Mbps
> RC parameters with bps = 2M
> -> some P frames, at 2Mbps
> Sequence parameters with bps = 3M
> -> IDR frame, at 3Mbps
> -> some P frames, at 3Mbps
The 2nd sequence is generated at 3Mbps with the current
logic. hl_bitrate_updated is 0 for your case and we use the sequence
parameters.
>
> But with the current code the second sequence is generated at 2Mbps
> because the RC parameters stay forever and "win" in some sense when
> they disagree.
>
> So, I think a nicer solution would be to add some way to determine
> which parameter set was provided most recently, so that we can always
> use the right one (with RC parameters overriding sequence parameters
> when both are provided). See patch below for a possible approach
> (rather ugly and not very well tested).
>
> Thanks,
>
> - Mark
>
>
> diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c
> index 15920f5..5d218d7 100644
> --- a/src/i965_drv_video.c
> +++ b/src/i965_drv_video.c
> @@ -3197,6 +3197,9 @@
> i965_encoder_render_misc_parameter_buffer(VADriverContextP ctx,
> if (param->type == VAEncMiscParameterTypeTemporalLayerStructure)
> encode->has_layers = 1;
>
> +if (param->type == VAEncMiscParameterTypeRateControl)
> +encode->frame_has_rc_parameters = 1;
> +
> index = i965_encoder_get_misc_paramerter_buffer_index(ctx,
> encode, param);
>
> if (index >= ARRAY_ELEMS(encode->misc_param[0]))
> @@ -3243,6 +3246,7 @@ i965_encoder_render_picture(VADriverContextP
> ctx,
>
> case VAEncSequenceParameterBufferType:
> vaStatus =
> I965_RENDER_ENCODE_BUFFER(sequence_parameter_ext);
> +encode->frame_has_sequence_parameters = 1;
> break;
>
> case VAEncPictureParameterBufferType:
> diff --git a/src/i965_drv_video.h b/src/i965_drv_video.h
> index 7cba3a3..b326e50 100644
> --- a/src/i965_drv_video.h
> +++ b/src/i965_drv_video.h
> @@ -272,6 +272,10 @@ struct encode_state
>
> int has_layers;
>
> +/* Whether these parameter sets were supplied with the current
> frame. */
> +int frame_has_rc_parameters;
> +int frame_has_sequence_parameters;
> +
> struct buffer_store *misc_param[16][8];
>
> VASurfaceID current_render_target;
> diff --git a/src/i965_encoder.c b/src/i965_encoder.c
> index d4d7445..0ae8965 100644
> --- a/src/i965_encoder.c
> +++ b/src/i965_encoder.c
> @@ -361,16 +361,20 @@
> intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
>
> if (num_pframes_in_gop != encoder_context-
> >brc.num_pframes_in_gop ||
> num_bframes_in_gop != encoder_context-
> >brc.num_bframes_in_gop ||
> -bits_per_second != encoder_context-
> >brc.bits_per_second[encoder_context->layer.num_layers - 1] ||
> framerate.num != encoder_context-
> >brc.framerate[encoder_context->layer.num_layers - 1].num ||
> framerate.den != encoder_context-
> >brc.framerate[encoder_context->layer.num_layers - 1].den) {
> encoder_context->brc.num_pframes_in_gop =
> num_pframes_in_gop;
> encoder_context->brc.num_bframes_in_gop =
> num_bframes_in_gop;
> -encoder_context->brc.bits_per_second[encoder_context-
> >layer.num_layers - 1] = bits_per_second;
> encoder_context->brc.framerate[encoder_c
Re: [Libva] [Libva-intel-driver][PATCH] Needn't reset brc if the bitrate setting isn't changed in the Begin/Render/End sequence
On 12/30/2016 02:52 AM, Mark Thompson wrote:
On 29/12/16 07:09, Zhao Yakui wrote:
On 12/29/2016 03:08 PM, Xiang, Haihao wrote:
On 12/29/2016 01:22 PM, Xiang, Haihao wrote:
User can use VAEncMiscParameterRateControl to update bitrate, so we
should ignore
the bitrate in the sequence parameter if
VAEncMiscParameterRateControl is present.
This makes sense. The MiscRateControl mentions that it can override
the
bps setting in sequence parameter.
Hence it is not needed to reset brc if
VAEncMiscParameterRateControl doesn't change
the used bitrate.
Does it still need to send the VAEncMiscParameterRateControl
parameter
if the bps is not changed for the new sequence?
Yes if bits_per_second in the sequence parameter is not equal to the
value for the previous Begin/Render/End sequence except bits_per_second
in the sequence parameter is 0.
OK.
It makes sense.
This looks good to me.
I'm not sure this behaviour is correct. Should not the most recently given
bitrate value from the user, either from sequence parameters or RC parameters,
be used? When is_new_sequence is set, the user will have provided a set of
sequence parameters (because we are making an IDR frame), so the bitrate there
is provided by the user and should override any previous RC parameters given
before that frame (but not any on this one).
That is, I think it should work like:
Hi, Mark
Thanks for patch review and nice consideration.
As we know, IDR frame can indicate that a new video sequence is
started. During the decoding, the previous frames before IDR are
discarded and not used. Maybe it will be better that the encoding
parameter related with RC is resent after a new sequence. That is to
say: The bps in SPS is used for the initialization for a new sequence.
If the MiscRC is sent, it will override the bps in SPS. If there is no
RC, the bps in SPS will be used.
In such case the user-space middleware only needs to follow the
same logic for every video sequence. Otherwise the different logic is
considered for each video sequence.
Not sure whether it makes sense?
Sequence parameters with bps = 1M
-> IDR frame, at 1Mbps
-> some P frames, at 1Mbps
RC parameters with bps = 2M
-> some P frames, at 2Mbps
Sequence parameters with bps = 3M
-> IDR frame, at 3Mbps
-> some P frames, at 3Mbps
But with the current code the second sequence is generated at 2Mbps because the RC
parameters stay forever and "win" in some sense when they disagree.
So, I think a nicer solution would be to add some way to determine which
parameter set was provided most recently, so that we can always use the right
one (with RC parameters overriding sequence parameters when both are provided).
See patch below for a possible approach (rather ugly and not very well tested).
Thanks,
- Mark
diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c
index 15920f5..5d218d7 100644
--- a/src/i965_drv_video.c
+++ b/src/i965_drv_video.c
@@ -3197,6 +3197,9 @@
i965_encoder_render_misc_parameter_buffer(VADriverContextP ctx,
if (param->type == VAEncMiscParameterTypeTemporalLayerStructure)
encode->has_layers = 1;
+if (param->type == VAEncMiscParameterTypeRateControl)
+encode->frame_has_rc_parameters = 1;
+
index = i965_encoder_get_misc_paramerter_buffer_index(ctx, encode, param);
if (index>= ARRAY_ELEMS(encode->misc_param[0]))
@@ -3243,6 +3246,7 @@ i965_encoder_render_picture(VADriverContextP ctx,
case VAEncSequenceParameterBufferType:
vaStatus = I965_RENDER_ENCODE_BUFFER(sequence_parameter_ext);
+encode->frame_has_sequence_parameters = 1;
break;
case VAEncPictureParameterBufferType:
diff --git a/src/i965_drv_video.h b/src/i965_drv_video.h
index 7cba3a3..b326e50 100644
--- a/src/i965_drv_video.h
+++ b/src/i965_drv_video.h
@@ -272,6 +272,10 @@ struct encode_state
int has_layers;
+/* Whether these parameter sets were supplied with the current frame. */
+int frame_has_rc_parameters;
+int frame_has_sequence_parameters;
+
struct buffer_store *misc_param[16][8];
VASurfaceID current_render_target;
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index d4d7445..0ae8965 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -361,16 +361,20 @@
intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop ||
num_bframes_in_gop != encoder_context->brc.num_bframes_in_gop ||
-bits_per_second !=
encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1] ||
framerate.num !=
encoder_context->brc.framerate[encoder_context->layer.num_layers - 1].num ||
framerate.den !=
encoder_context->brc.framerate[encoder_context->layer.num_layers - 1].den) {
encoder_context->brc.num_pframes_in_gop = num_pframes_in_gop;
encoder_context->brc.num_bframes_in_gop = num_bfr
Re: [Libva] [Libva-intel-driver][PATCH] Needn't reset brc if the bitrate setting isn't changed in the Begin/Render/End sequence
On 29/12/16 07:09, Zhao Yakui wrote:
> On 12/29/2016 03:08 PM, Xiang, Haihao wrote:
>>
>>> On 12/29/2016 01:22 PM, Xiang, Haihao wrote:
User can use VAEncMiscParameterRateControl to update bitrate, so we
should ignore
the bitrate in the sequence parameter if
VAEncMiscParameterRateControl is present.
>>>
>>> This makes sense. The MiscRateControl mentions that it can override
>>> the
>>> bps setting in sequence parameter.
>>>
Hence it is not needed to reset brc if
VAEncMiscParameterRateControl doesn't change
the used bitrate.
>>>
>>> Does it still need to send the VAEncMiscParameterRateControl
>>> parameter
>>> if the bps is not changed for the new sequence?
>>
>> Yes if bits_per_second in the sequence parameter is not equal to the
>> value for the previous Begin/Render/End sequence except bits_per_second
>> in the sequence parameter is 0.
>>
>
> OK.
> It makes sense.
>
> This looks good to me.
I'm not sure this behaviour is correct. Should not the most recently given
bitrate value from the user, either from sequence parameters or RC parameters,
be used? When is_new_sequence is set, the user will have provided a set of
sequence parameters (because we are making an IDR frame), so the bitrate there
is provided by the user and should override any previous RC parameters given
before that frame (but not any on this one).
That is, I think it should work like:
Sequence parameters with bps = 1M
-> IDR frame, at 1Mbps
-> some P frames, at 1Mbps
RC parameters with bps = 2M
-> some P frames, at 2Mbps
Sequence parameters with bps = 3M
-> IDR frame, at 3Mbps
-> some P frames, at 3Mbps
But with the current code the second sequence is generated at 2Mbps because the
RC parameters stay forever and "win" in some sense when they disagree.
So, I think a nicer solution would be to add some way to determine which
parameter set was provided most recently, so that we can always use the right
one (with RC parameters overriding sequence parameters when both are provided).
See patch below for a possible approach (rather ugly and not very well tested).
Thanks,
- Mark
diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c
index 15920f5..5d218d7 100644
--- a/src/i965_drv_video.c
+++ b/src/i965_drv_video.c
@@ -3197,6 +3197,9 @@
i965_encoder_render_misc_parameter_buffer(VADriverContextP ctx,
if (param->type == VAEncMiscParameterTypeTemporalLayerStructure)
encode->has_layers = 1;
+if (param->type == VAEncMiscParameterTypeRateControl)
+encode->frame_has_rc_parameters = 1;
+
index = i965_encoder_get_misc_paramerter_buffer_index(ctx, encode, param);
if (index >= ARRAY_ELEMS(encode->misc_param[0]))
@@ -3243,6 +3246,7 @@ i965_encoder_render_picture(VADriverContextP ctx,
case VAEncSequenceParameterBufferType:
vaStatus = I965_RENDER_ENCODE_BUFFER(sequence_parameter_ext);
+encode->frame_has_sequence_parameters = 1;
break;
case VAEncPictureParameterBufferType:
diff --git a/src/i965_drv_video.h b/src/i965_drv_video.h
index 7cba3a3..b326e50 100644
--- a/src/i965_drv_video.h
+++ b/src/i965_drv_video.h
@@ -272,6 +272,10 @@ struct encode_state
int has_layers;
+/* Whether these parameter sets were supplied with the current frame. */
+int frame_has_rc_parameters;
+int frame_has_sequence_parameters;
+
struct buffer_store *misc_param[16][8];
VASurfaceID current_render_target;
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index d4d7445..0ae8965 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -361,16 +361,20 @@
intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop ||
num_bframes_in_gop != encoder_context->brc.num_bframes_in_gop ||
-bits_per_second !=
encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1] ||
framerate.num !=
encoder_context->brc.framerate[encoder_context->layer.num_layers - 1].num ||
framerate.den !=
encoder_context->brc.framerate[encoder_context->layer.num_layers - 1].den) {
encoder_context->brc.num_pframes_in_gop = num_pframes_in_gop;
encoder_context->brc.num_bframes_in_gop = num_bframes_in_gop;
-encoder_context->brc.bits_per_second[encoder_context->layer.num_layers
- 1] = bits_per_second;
encoder_context->brc.framerate[encoder_context->layer.num_layers - 1]
= framerate;
encoder_context->brc.need_reset = 1;
}
+if (encode_state->frame_has_sequence_parameters &&
!encode_state->frame_has_rc_parameters &&
+bits_per_second !=
encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
+encoder_context->brc.bits_per_second[encoder_context->layer.num_layers
- 1] = bits_per_second;
+encoder_context->brc.need_reset = 1;
+}
+
if (!encoder_context->brc.hrd_buffer_size ||
!encoder
