Discussion:
[Libva-intel-driver][PATCH] Needn't reset brc if the bitrate setting isn't changed in the Begin/Render/End sequence
(too old to reply)
Xiang, Haihao
2016-12-29 05:22:35 UTC
Permalink
User can use VAEncMiscParameterRateControl to update bitrate, so we should ignore
the bitrate in the sequence parameter if VAEncMiscParameterRateControl is present.
Hence it is not needed to reset brc if VAEncMiscParameterRateControl doesn't change
the used bitrate.

Signed-off-by: Xiang, Haihao <***@intel.com>
---
src/i965_encoder.c | 72 +++++++++++++++++++++++++++++++++---------------------
1 file changed, 44 insertions(+), 28 deletions(-)

diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index d4d7445..ae31f01 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -314,18 +314,17 @@ intel_encoder_check_jpeg_yuv_surface(VADriverContextP ctx,
static VAStatus
intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
struct encode_state *encode_state,
- struct intel_encoder_context *encoder_context)
+ struct intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
VAEncSequenceParameterBufferH264 *seq_param = (VAEncSequenceParameterBufferH264 *)encode_state->seq_param_ext->buffer;
struct intel_fraction framerate;
- unsigned int bits_per_second;
unsigned short num_pframes_in_gop, num_bframes_in_gop;

if (!encoder_context->is_new_sequence)
return VA_STATUS_SUCCESS;

assert(seq_param);
- bits_per_second = seq_param->bits_per_second; // for the highest layer

if (!seq_param->num_units_in_tick || !seq_param->time_scale) {
framerate = (struct intel_fraction) { 30, 1 };
@@ -361,12 +360,10 @@ 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;
}
@@ -377,6 +374,8 @@ intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.hrd_initial_buffer_fullness = seq_param->bits_per_second;
}

+ *seq_bits_per_second = seq_param->bits_per_second;
+
return VA_STATUS_SUCCESS;

error:
@@ -386,10 +385,11 @@ error:
static VAStatus
intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
struct encode_state *encode_state,
- struct intel_encoder_context *encoder_context)
+ struct intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
VAEncSequenceParameterBufferVP8 *seq_param = (VAEncSequenceParameterBufferVP8 *)encode_state->seq_param_ext->buffer;
- unsigned int num_pframes_in_gop, bits_per_second;
+ unsigned int num_pframes_in_gop;

if (!encoder_context->is_new_sequence)
return VA_STATUS_SUCCESS;
@@ -406,7 +406,6 @@ intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
}

num_pframes_in_gop = encoder_context->brc.gop_size - 1;
- bits_per_second = seq_param->bits_per_second; // for the highest layer

if (!encoder_context->brc.framerate[encoder_context->layer.num_layers - 1].num) {
// for the highest layer
@@ -414,10 +413,8 @@ intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}

- if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop ||
- bits_per_second != encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
+ if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop) {
encoder_context->brc.num_pframes_in_gop = num_pframes_in_gop;
- encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1] = bits_per_second;
encoder_context->brc.need_reset = 1;
}

@@ -428,13 +425,16 @@ intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}

+ *seq_bits_per_second = seq_param->bits_per_second;
+
return VA_STATUS_SUCCESS;
}

static VAStatus
intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
struct encode_state *encode_state,
- struct intel_encoder_context *encoder_context)
+ struct intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
VAEncSequenceParameterBufferHEVC *seq_param = (VAEncSequenceParameterBufferHEVC*)encode_state->seq_param_ext->buffer;
struct intel_fraction framerate;
@@ -481,10 +481,7 @@ intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}

- if (encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second) {
- encoder_context->brc.bits_per_second[0] = seq_param->bits_per_second;
- encoder_context->brc.need_reset = 1;
- }
+ *seq_bits_per_second = seq_param->bits_per_second;

return VA_STATUS_SUCCESS;
}
@@ -492,7 +489,8 @@ intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
static VAStatus
intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP ctx,
struct encode_state *encode_state,
- struct intel_encoder_context *encoder_context)
+ struct intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
VAEncSequenceParameterBufferVP9 *seq_param = (VAEncSequenceParameterBufferVP9*)encode_state->seq_param_ext->buffer;
unsigned int gop_size;
@@ -507,34 +505,37 @@ intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP ctx,
else
gop_size = seq_param->intra_period;

- if (encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second ||
- encoder_context->brc.gop_size != gop_size) {
- encoder_context->brc.bits_per_second[0] = seq_param->bits_per_second;
+ if (encoder_context->brc.gop_size != gop_size) {
encoder_context->brc.gop_size = gop_size;
encoder_context->brc.need_reset = 1;
}

+ *seq_bits_per_second = seq_param->bits_per_second;
+
return VA_STATUS_SUCCESS;
}

static VAStatus
intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,
struct encode_state *encode_state,
- struct intel_encoder_context *encoder_context)
+ struct intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
+ *seq_bits_per_second = 0;
+
switch (encoder_context->codec) {
case CODEC_H264:
case CODEC_H264_MVC:
- return intel_encoder_check_brc_h264_sequence_parameter(ctx, encode_state, encoder_context);
+ return intel_encoder_check_brc_h264_sequence_parameter(ctx, encode_state, encoder_context, seq_bits_per_second);

case CODEC_VP8:
- return intel_encoder_check_brc_vp8_sequence_parameter(ctx, encode_state, encoder_context);
+ return intel_encoder_check_brc_vp8_sequence_parameter(ctx, encode_state, encoder_context, seq_bits_per_second);

case CODEC_HEVC:
- return intel_encoder_check_brc_hevc_sequence_parameter(ctx, encode_state, encoder_context);
+ return intel_encoder_check_brc_hevc_sequence_parameter(ctx, encode_state, encoder_context, seq_bits_per_second);

case CODEC_VP9:
- return intel_encoder_check_brc_vp9_sequence_parameter(ctx, encode_state, encoder_context);
+ return intel_encoder_check_brc_vp9_sequence_parameter(ctx, encode_state, encoder_context, seq_bits_per_second);

default:
// TODO: other codecs
@@ -545,7 +546,8 @@ intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,
static void
intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
struct intel_encoder_context *encoder_context,
- VAEncMiscParameterRateControl *misc)
+ VAEncMiscParameterRateControl *misc,
+ int *hl_bitrate_updated)
{
int temporal_id = 0;

@@ -577,6 +579,9 @@ intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
encoder_context->brc.window_size = misc->window_size;
encoder_context->brc.need_reset = 1;
}
+
+ if (temporal_id == encoder_context->layer.num_layers - 1)
+ *hl_bitrate_updated = 1;
}

static void
@@ -651,11 +656,13 @@ intel_encoder_check_brc_parameter(VADriverContextP ctx,
VAStatus ret;
VAEncMiscParameterBuffer *misc_param;
int i, j;
+ int hl_bitrate_updated = 0; // Indicate whether the bitrate for the highest level is changed in misc parameters
+ unsigned int seq_bits_per_second = 0;

if (!(encoder_context->rate_control_mode & (VA_RC_CBR | VA_RC_VBR)))
return VA_STATUS_SUCCESS;

- ret = intel_encoder_check_brc_sequence_parameter(ctx, encode_state, encoder_context);
+ ret = intel_encoder_check_brc_sequence_parameter(ctx, encode_state, encoder_context, &seq_bits_per_second);

if (ret)
return ret;
@@ -677,7 +684,8 @@ intel_encoder_check_brc_parameter(VADriverContextP ctx,
case VAEncMiscParameterTypeRateControl:
intel_encoder_check_rate_control_parameter(ctx,
encoder_context,
- (VAEncMiscParameterRateControl *)misc_param->data);
+ (VAEncMiscParameterRateControl *)misc_param->data,
+ &hl_bitrate_updated);
break;

case VAEncMiscParameterTypeHRD:
@@ -698,6 +706,14 @@ intel_encoder_check_brc_parameter(VADriverContextP ctx,
}
}

+ if (!hl_bitrate_updated && seq_bits_per_second &&
+ encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1] != seq_bits_per_second) {
+
+ encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1] = seq_bits_per_second;
+ encoder_context->brc.need_reset = 1;
+
+ }
+
return VA_STATUS_SUCCESS;
}
--
1.9.1
Zhao Yakui
2016-12-29 06:14:24 UTC
Permalink
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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?

Thanks
Yakui
Post by Xiang, Haihao
---
src/i965_encoder.c | 72 +++++++++++++++++++++++++++++++++---------------------
1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index d4d7445..ae31f01 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -314,18 +314,17 @@ intel_encoder_check_jpeg_yuv_surface(VADriverContextP ctx,
static VAStatus
intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
struct encode_state *encode_state,
- struct intel_encoder_context *encoder_context)
+ struct intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
VAEncSequenceParameterBufferH264 *seq_param = (VAEncSequenceParameterBufferH264 *)encode_state->seq_param_ext->buffer;
struct intel_fraction framerate;
- unsigned int bits_per_second;
unsigned short num_pframes_in_gop, num_bframes_in_gop;
if (!encoder_context->is_new_sequence)
return VA_STATUS_SUCCESS;
assert(seq_param);
- bits_per_second = seq_param->bits_per_second; // for the highest layer
if (!seq_param->num_units_in_tick || !seq_param->time_scale) {
framerate = (struct intel_fraction) { 30, 1 };
@@ -361,12 +360,10 @@ 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;
}
@@ -377,6 +374,8 @@ intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.hrd_initial_buffer_fullness = seq_param->bits_per_second;
}
+ *seq_bits_per_second = seq_param->bits_per_second;
+
return VA_STATUS_SUCCESS;
static VAStatus
intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
struct encode_state *encode_state,
- struct intel_encoder_context *encoder_context)
+ struct intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
VAEncSequenceParameterBufferVP8 *seq_param = (VAEncSequenceParameterBufferVP8 *)encode_state->seq_param_ext->buffer;
- unsigned int num_pframes_in_gop, bits_per_second;
+ unsigned int num_pframes_in_gop;
if (!encoder_context->is_new_sequence)
return VA_STATUS_SUCCESS;
@@ -406,7 +406,6 @@ intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
}
num_pframes_in_gop = encoder_context->brc.gop_size - 1;
- bits_per_second = seq_param->bits_per_second; // for the highest layer
if (!encoder_context->brc.framerate[encoder_context->layer.num_layers - 1].num) {
// for the highest layer
@@ -414,10 +413,8 @@ intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}
- if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop ||
- bits_per_second != encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
+ if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop) {
encoder_context->brc.num_pframes_in_gop = num_pframes_in_gop;
- encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1] = bits_per_second;
encoder_context->brc.need_reset = 1;
}
@@ -428,13 +425,16 @@ intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}
+ *seq_bits_per_second = seq_param->bits_per_second;
+
return VA_STATUS_SUCCESS;
}
static VAStatus
intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
struct encode_state *encode_state,
- struct intel_encoder_context *encoder_context)
+ struct intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
VAEncSequenceParameterBufferHEVC *seq_param = (VAEncSequenceParameterBufferHEVC*)encode_state->seq_param_ext->buffer;
struct intel_fraction framerate;
@@ -481,10 +481,7 @@ intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}
- if (encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second) {
- encoder_context->brc.bits_per_second[0] = seq_param->bits_per_second;
- encoder_context->brc.need_reset = 1;
- }
+ *seq_bits_per_second = seq_param->bits_per_second;
return VA_STATUS_SUCCESS;
}
@@ -492,7 +489,8 @@ intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
static VAStatus
intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP ctx,
struct encode_state *encode_state,
- struct intel_encoder_context *encoder_context)
+ struct intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
VAEncSequenceParameterBufferVP9 *seq_param = (VAEncSequenceParameterBufferVP9*)encode_state->seq_param_ext->buffer;
unsigned int gop_size;
@@ -507,34 +505,37 @@ intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP ctx,
else
gop_size = seq_param->intra_period;
- if (encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second ||
- encoder_context->brc.gop_size != gop_size) {
- encoder_context->brc.bits_per_second[0] = seq_param->bits_per_second;
+ if (encoder_context->brc.gop_size != gop_size) {
encoder_context->brc.gop_size = gop_size;
encoder_context->brc.need_reset = 1;
}
+ *seq_bits_per_second = seq_param->bits_per_second;
+
return VA_STATUS_SUCCESS;
}
static VAStatus
intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,
struct encode_state *encode_state,
- struct intel_encoder_context *encoder_context)
+ struct intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
+ *seq_bits_per_second = 0;
+
switch (encoder_context->codec) {
- return intel_encoder_check_brc_h264_sequence_parameter(ctx, encode_state, encoder_context);
+ return intel_encoder_check_brc_h264_sequence_parameter(ctx, encode_state, encoder_context, seq_bits_per_second);
- return intel_encoder_check_brc_vp8_sequence_parameter(ctx, encode_state, encoder_context);
+ return intel_encoder_check_brc_vp8_sequence_parameter(ctx, encode_state, encoder_context, seq_bits_per_second);
- return intel_encoder_check_brc_hevc_sequence_parameter(ctx, encode_state, encoder_context);
+ return intel_encoder_check_brc_hevc_sequence_parameter(ctx, encode_state, encoder_context, seq_bits_per_second);
- return intel_encoder_check_brc_vp9_sequence_parameter(ctx, encode_state, encoder_context);
+ return intel_encoder_check_brc_vp9_sequence_parameter(ctx, encode_state, encoder_context, seq_bits_per_second);
// TODO: other codecs
@@ -545,7 +546,8 @@ intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,
static void
intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
struct intel_encoder_context *encoder_context,
- VAEncMiscParameterRateControl *misc)
+ VAEncMiscParameterRateControl *misc,
+ int *hl_bitrate_updated)
{
int temporal_id = 0;
@@ -577,6 +579,9 @@ intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
encoder_context->brc.window_size = misc->window_size;
encoder_context->brc.need_reset = 1;
}
+
+ if (temporal_id == encoder_context->layer.num_layers - 1)
+ *hl_bitrate_updated = 1;
}
static void
@@ -651,11 +656,13 @@ intel_encoder_check_brc_parameter(VADriverContextP ctx,
VAStatus ret;
VAEncMiscParameterBuffer *misc_param;
int i, j;
+ int hl_bitrate_updated = 0; // Indicate whether the bitrate for the highest level is changed in misc parameters
+ unsigned int seq_bits_per_second = 0;
if (!(encoder_context->rate_control_mode& (VA_RC_CBR | VA_RC_VBR)))
return VA_STATUS_SUCCESS;
- ret = intel_encoder_check_brc_sequence_parameter(ctx, encode_state, encoder_context);
+ ret = intel_encoder_check_brc_sequence_parameter(ctx, encode_state, encoder_context,&seq_bits_per_second);
if (ret)
return ret;
@@ -677,7 +684,8 @@ intel_encoder_check_brc_parameter(VADriverContextP ctx,
intel_encoder_check_rate_control_parameter(ctx,
encoder_context,
- (VAEncMiscParameterRateControl *)misc_param->data);
+ (VAEncMiscParameterRateControl *)misc_param->data,
+&hl_bitrate_updated);
break;
@@ -698,6 +706,14 @@ intel_encoder_check_brc_parameter(VADriverContextP ctx,
}
}
+ if (!hl_bitrate_updated&& seq_bits_per_second&&
+ encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1] != seq_bits_per_second) {
+
+ encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1] = seq_bits_per_second;
+ encoder_context->brc.need_reset = 1;
+
+ }
+
return VA_STATUS_SUCCESS;
}
Xiang, Haihao
2016-12-29 07:08:10 UTC
Permalink
Post by Zhao Yakui
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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. 
Post by Zhao Yakui
Thanks
   Yakui
Post by Xiang, Haihao
---
  src/i965_encoder.c | 72 +++++++++++++++++++++++++++++++++------
---------------
  1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index d4d7445..ae31f01 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -314,18 +314,17 @@
intel_encoder_check_jpeg_yuv_surface(VADriverContextP ctx,
  static VAStatus
  intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP
ctx,
                                                  struct
encode_state *encode_state,
-                                                struct
intel_encoder_context *encoder_context)
+                                                struct
intel_encoder_context *encoder_context,
+                                                unsigned int
*seq_bits_per_second)
  {
      VAEncSequenceParameterBufferH264 *seq_param =
(VAEncSequenceParameterBufferH264 *)encode_state->seq_param_ext-
Post by Xiang, Haihao
buffer;
      struct intel_fraction framerate;
-    unsigned int bits_per_second;
      unsigned short num_pframes_in_gop, num_bframes_in_gop;
      if (!encoder_context->is_new_sequence)
          return VA_STATUS_SUCCESS;
      assert(seq_param);
-    bits_per_second = seq_param->bits_per_second; // for the
highest layer
      if (!seq_param->num_units_in_tick || !seq_param->time_scale)
{
          framerate = (struct intel_fraction) { 30, 1 };
@@ -361,12 +360,10 @@
intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
      if (num_pframes_in_gop != encoder_context-
Post by Xiang, Haihao
brc.num_pframes_in_gop ||
          num_bframes_in_gop != encoder_context-
Post by Xiang, Haihao
brc.num_bframes_in_gop ||
-        bits_per_second != encoder_context-
Post by Xiang, Haihao
brc.bits_per_second[encoder_context->layer.num_layers - 1] ||
          framerate.num != encoder_context-
Post by Xiang, Haihao
brc.framerate[encoder_context->layer.num_layers - 1].num ||
          framerate.den != encoder_context-
Post by Xiang, Haihao
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-
Post by Xiang, Haihao
layer.num_layers - 1] = bits_per_second;
          encoder_context->brc.framerate[encoder_context-
Post by Xiang, Haihao
layer.num_layers - 1] = framerate;
          encoder_context->brc.need_reset = 1;
      }
@@ -377,6 +374,8 @@
intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
          encoder_context->brc.hrd_initial_buffer_fullness =
seq_param->bits_per_second;
      }
+    *seq_bits_per_second = seq_param->bits_per_second;
+
      return VA_STATUS_SUCCESS;
  static VAStatus
  intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP
ctx,
                                                 struct
encode_state *encode_state,
-                                               struct
intel_encoder_context *encoder_context)
+                                               struct
intel_encoder_context *encoder_context,
+                                               unsigned int
*seq_bits_per_second)
  {
      VAEncSequenceParameterBufferVP8 *seq_param =
(VAEncSequenceParameterBufferVP8 *)encode_state->seq_param_ext-
Post by Xiang, Haihao
buffer;
-    unsigned int num_pframes_in_gop, bits_per_second;
+    unsigned int num_pframes_in_gop;
      if (!encoder_context->is_new_sequence)
          return VA_STATUS_SUCCESS;
@@ -406,7 +406,6 @@
intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
      }
      num_pframes_in_gop = encoder_context->brc.gop_size - 1;
-    bits_per_second = seq_param->bits_per_second;       // for the
highest layer
      if (!encoder_context->brc.framerate[encoder_context-
Post by Xiang, Haihao
layer.num_layers - 1].num) {
          // for the highest layer
@@ -414,10 +413,8 @@
intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
          encoder_context->brc.need_reset = 1;
      }
-    if (num_pframes_in_gop != encoder_context-
Post by Xiang, Haihao
brc.num_pframes_in_gop ||
-        bits_per_second != encoder_context-
Post by Xiang, Haihao
brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
+    if (num_pframes_in_gop != encoder_context-
Post by Xiang, Haihao
brc.num_pframes_in_gop) {
          encoder_context->brc.num_pframes_in_gop =
num_pframes_in_gop;
-        encoder_context->brc.bits_per_second[encoder_context-
Post by Xiang, Haihao
layer.num_layers - 1] = bits_per_second;
          encoder_context->brc.need_reset = 1;
      }
@@ -428,13 +425,16 @@
intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
          encoder_context->brc.need_reset = 1;
      }
+    *seq_bits_per_second = seq_param->bits_per_second;
+
      return VA_STATUS_SUCCESS;
  }
  static VAStatus
  intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP
ctx,
                                                  struct
encode_state *encode_state,
-                                                struct
intel_encoder_context *encoder_context)
+                                                struct
intel_encoder_context *encoder_context,
+                                                unsigned int
*seq_bits_per_second)
  {
      VAEncSequenceParameterBufferHEVC *seq_param =
(VAEncSequenceParameterBufferHEVC*)encode_state->seq_param_ext-
Post by Xiang, Haihao
buffer;
      struct intel_fraction framerate;
@@ -481,10 +481,7 @@
intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
          encoder_context->brc.need_reset = 1;
      }
-    if (encoder_context->brc.bits_per_second[0] != seq_param-
Post by Xiang, Haihao
bits_per_second) {
-        encoder_context->brc.bits_per_second[0] = seq_param-
Post by Xiang, Haihao
bits_per_second;
-        encoder_context->brc.need_reset = 1;
-    }
+    *seq_bits_per_second = seq_param->bits_per_second;
      return VA_STATUS_SUCCESS;
  }
@@ -492,7 +489,8 @@
intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
  static VAStatus
  intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP
ctx,
                                                 struct
encode_state *encode_state,
-                                               struct
intel_encoder_context *encoder_context)
+                                               struct
intel_encoder_context *encoder_context,
+                                               unsigned int
*seq_bits_per_second)
  {
      VAEncSequenceParameterBufferVP9 *seq_param =
(VAEncSequenceParameterBufferVP9*)encode_state->seq_param_ext-
Post by Xiang, Haihao
buffer;
      unsigned int gop_size;
@@ -507,34 +505,37 @@
intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP ctx,
      else
          gop_size = seq_param->intra_period;
-    if (encoder_context->brc.bits_per_second[0] != seq_param-
Post by Xiang, Haihao
bits_per_second ||
-        encoder_context->brc.gop_size != gop_size) {
-        encoder_context->brc.bits_per_second[0] = seq_param-
Post by Xiang, Haihao
bits_per_second;
+    if (encoder_context->brc.gop_size != gop_size) {
          encoder_context->brc.gop_size = gop_size;
          encoder_context->brc.need_reset = 1;
      }
+    *seq_bits_per_second = seq_param->bits_per_second;
+
      return VA_STATUS_SUCCESS;
  }
  static VAStatus
  intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,
                                             struct encode_state
*encode_state,
-                                           struct
intel_encoder_context *encoder_context)
+                                           struct
intel_encoder_context *encoder_context,
+                                           unsigned int
*seq_bits_per_second)
  {
+    *seq_bits_per_second = 0;
+
      switch (encoder_context->codec) {
-        return
intel_encoder_check_brc_h264_sequence_parameter(ctx, encode_state,
encoder_context);
+        return
intel_encoder_check_brc_h264_sequence_parameter(ctx, encode_state,
encoder_context, seq_bits_per_second);
-        return intel_encoder_check_brc_vp8_sequence_parameter(ctx,
encode_state, encoder_context);
+        return intel_encoder_check_brc_vp8_sequence_parameter(ctx,
encode_state, encoder_context, seq_bits_per_second);
-        return
intel_encoder_check_brc_hevc_sequence_parameter(ctx, encode_state,
encoder_context);
+        return
intel_encoder_check_brc_hevc_sequence_parameter(ctx, encode_state,
encoder_context, seq_bits_per_second);
-        return intel_encoder_check_brc_vp9_sequence_parameter(ctx,
encode_state, encoder_context);
+        return intel_encoder_check_brc_vp9_sequence_parameter(ctx,
encode_state, encoder_context, seq_bits_per_second);
          // TODO: other codecs
@@ -545,7 +546,8 @@
intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,
  static void
  intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
                                             struct
intel_encoder_context *encoder_context,
-                                           VAEncMiscParameterRateC
ontrol *misc)
+                                           VAEncMiscParameterRateC
ontrol *misc,
+                                           int
*hl_bitrate_updated)
  {
      int temporal_id = 0;
@@ -577,6 +579,9 @@
intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
          encoder_context->brc.window_size = misc->window_size;
          encoder_context->brc.need_reset = 1;
      }
+
+    if (temporal_id == encoder_context->layer.num_layers - 1)
+        *hl_bitrate_updated = 1;
  }
  static void
@@ -651,11 +656,13 @@
intel_encoder_check_brc_parameter(VADriverContextP ctx,
      VAStatus ret;
      VAEncMiscParameterBuffer *misc_param;
      int i, j;
+    int hl_bitrate_updated = 0; // Indicate whether the bitrate
for the highest level is changed in misc parameters
+    unsigned int seq_bits_per_second = 0;
      if (!(encoder_context->rate_control_mode&  (VA_RC_CBR |
VA_RC_VBR)))
          return VA_STATUS_SUCCESS;
-    ret = intel_encoder_check_brc_sequence_parameter(ctx,
encode_state, encoder_context);
+    ret = intel_encoder_check_brc_sequence_parameter(ctx,
encode_state, encoder_context,&seq_bits_per_second);
      if (ret)
          return ret;
@@ -677,7 +684,8 @@
intel_encoder_check_brc_parameter(VADriverContextP ctx,
                  intel_encoder_check_rate_control_parameter(ctx,
                                                             encode
r_context,
-                                                           (VAEncM
iscParameterRateControl *)misc_param->data);
+                                                           (VAEncM
iscParameterRateControl *)misc_param->data,
+&hl_bitrate_updated);
                  break;
@@ -698,6 +706,14 @@
intel_encoder_check_brc_parameter(VADriverContextP ctx,
          }
      }
+    if (!hl_bitrate_updated&&  seq_bits_per_second&&
+        encoder_context->brc.bits_per_second[encoder_context-
Post by Xiang, Haihao
layer.num_layers - 1] != seq_bits_per_second) {
+
+        encoder_context->brc.bits_per_second[encoder_context-
Post by Xiang, Haihao
layer.num_layers - 1] = seq_bits_per_second;
+        encoder_context->brc.need_reset = 1;
+
+    }
+
      return VA_STATUS_SUCCESS;
  }
Zhao Yakui
2016-12-29 07:09:34 UTC
Permalink
Post by Xiang, Haihao
Post by Zhao Yakui
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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.

Add: Reviewed-by: Zhao Yakui <***@intel.com>

Thanks
Yakui
Post by Xiang, Haihao
Post by Zhao Yakui
Thanks
Yakui
Post by Xiang, Haihao
---
src/i965_encoder.c | 72 +++++++++++++++++++++++++++++++++------
---------------
1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index d4d7445..ae31f01 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -314,18 +314,17 @@
intel_encoder_check_jpeg_yuv_surface(VADriverContextP ctx,
static VAStatus
intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
struct
encode_state *encode_state,
- struct
intel_encoder_context *encoder_context)
+ struct
intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
VAEncSequenceParameterBufferH264 *seq_param =
(VAEncSequenceParameterBufferH264 *)encode_state->seq_param_ext-
Post by Xiang, Haihao
buffer;
struct intel_fraction framerate;
- unsigned int bits_per_second;
unsigned short num_pframes_in_gop, num_bframes_in_gop;
if (!encoder_context->is_new_sequence)
return VA_STATUS_SUCCESS;
assert(seq_param);
- bits_per_second = seq_param->bits_per_second; // for the highest layer
if (!seq_param->num_units_in_tick || !seq_param->time_scale) {
framerate = (struct intel_fraction) { 30, 1 };
@@ -361,12 +360,10 @@
intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
if (num_pframes_in_gop != encoder_context-
Post by Xiang, Haihao
brc.num_pframes_in_gop ||
num_bframes_in_gop != encoder_context-
Post by Xiang, Haihao
brc.num_bframes_in_gop ||
- bits_per_second != encoder_context-
Post by Xiang, Haihao
brc.bits_per_second[encoder_context->layer.num_layers - 1] ||
framerate.num != encoder_context-
Post by Xiang, Haihao
brc.framerate[encoder_context->layer.num_layers - 1].num ||
framerate.den != encoder_context-
Post by Xiang, Haihao
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-
Post by Xiang, Haihao
layer.num_layers - 1] = bits_per_second;
encoder_context->brc.framerate[encoder_context-
Post by Xiang, Haihao
layer.num_layers - 1] = framerate;
encoder_context->brc.need_reset = 1;
}
@@ -377,6 +374,8 @@
intel_encoder_check_brc_h264_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.hrd_initial_buffer_fullness =
seq_param->bits_per_second;
}
+ *seq_bits_per_second = seq_param->bits_per_second;
+
return VA_STATUS_SUCCESS;
static VAStatus
intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
struct
encode_state *encode_state,
- struct
intel_encoder_context *encoder_context)
+ struct
intel_encoder_context *encoder_context,
+ unsigned int
*seq_bits_per_second)
{
VAEncSequenceParameterBufferVP8 *seq_param =
(VAEncSequenceParameterBufferVP8 *)encode_state->seq_param_ext-
Post by Xiang, Haihao
buffer;
- unsigned int num_pframes_in_gop, bits_per_second;
+ unsigned int num_pframes_in_gop;
if (!encoder_context->is_new_sequence)
return VA_STATUS_SUCCESS;
@@ -406,7 +406,6 @@
intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
}
num_pframes_in_gop = encoder_context->brc.gop_size - 1;
- bits_per_second = seq_param->bits_per_second; // for the highest layer
if (!encoder_context->brc.framerate[encoder_context-
Post by Xiang, Haihao
layer.num_layers - 1].num) {
// for the highest layer
@@ -414,10 +413,8 @@
intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}
- if (num_pframes_in_gop != encoder_context-
Post by Xiang, Haihao
brc.num_pframes_in_gop ||
- bits_per_second != encoder_context-
Post by Xiang, Haihao
brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
+ if (num_pframes_in_gop != encoder_context-
Post by Xiang, Haihao
brc.num_pframes_in_gop) {
encoder_context->brc.num_pframes_in_gop =
num_pframes_in_gop;
- encoder_context->brc.bits_per_second[encoder_context-
Post by Xiang, Haihao
layer.num_layers - 1] = bits_per_second;
encoder_context->brc.need_reset = 1;
}
@@ -428,13 +425,16 @@
intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}
+ *seq_bits_per_second = seq_param->bits_per_second;
+
return VA_STATUS_SUCCESS;
}
static VAStatus
intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
struct
encode_state *encode_state,
- struct
intel_encoder_context *encoder_context)
+ struct
intel_encoder_context *encoder_context,
+ unsigned int *seq_bits_per_second)
{
VAEncSequenceParameterBufferHEVC *seq_param =
(VAEncSequenceParameterBufferHEVC*)encode_state->seq_param_ext-
Post by Xiang, Haihao
buffer;
struct intel_fraction framerate;
@@ -481,10 +481,7 @@
intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}
- if (encoder_context->brc.bits_per_second[0] != seq_param-
Post by Xiang, Haihao
bits_per_second) {
- encoder_context->brc.bits_per_second[0] = seq_param-
Post by Xiang, Haihao
bits_per_second;
- encoder_context->brc.need_reset = 1;
- }
+ *seq_bits_per_second = seq_param->bits_per_second;
return VA_STATUS_SUCCESS;
}
@@ -492,7 +489,8 @@
intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
static VAStatus
intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP ctx,
struct
encode_state *encode_state,
- struct
intel_encoder_context *encoder_context)
+ struct
intel_encoder_context *encoder_context,
+ unsigned int
*seq_bits_per_second)
{
VAEncSequenceParameterBufferVP9 *seq_param =
(VAEncSequenceParameterBufferVP9*)encode_state->seq_param_ext-
Post by Xiang, Haihao
buffer;
unsigned int gop_size;
@@ -507,34 +505,37 @@
intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP ctx,
else
gop_size = seq_param->intra_period;
- if (encoder_context->brc.bits_per_second[0] != seq_param-
Post by Xiang, Haihao
bits_per_second ||
- encoder_context->brc.gop_size != gop_size) {
- encoder_context->brc.bits_per_second[0] = seq_param-
Post by Xiang, Haihao
bits_per_second;
+ if (encoder_context->brc.gop_size != gop_size) {
encoder_context->brc.gop_size = gop_size;
encoder_context->brc.need_reset = 1;
}
+ *seq_bits_per_second = seq_param->bits_per_second;
+
return VA_STATUS_SUCCESS;
}
static VAStatus
intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,
struct encode_state *encode_state,
- struct
intel_encoder_context *encoder_context)
+ struct
intel_encoder_context *encoder_context,
+ unsigned int
*seq_bits_per_second)
{
+ *seq_bits_per_second = 0;
+
switch (encoder_context->codec) {
- return
intel_encoder_check_brc_h264_sequence_parameter(ctx, encode_state,
encoder_context);
+ return
intel_encoder_check_brc_h264_sequence_parameter(ctx, encode_state,
encoder_context, seq_bits_per_second);
- return intel_encoder_check_brc_vp8_sequence_parameter(ctx,
encode_state, encoder_context);
+ return intel_encoder_check_brc_vp8_sequence_parameter(ctx,
encode_state, encoder_context, seq_bits_per_second);
- return
intel_encoder_check_brc_hevc_sequence_parameter(ctx, encode_state,
encoder_context);
+ return
intel_encoder_check_brc_hevc_sequence_parameter(ctx, encode_state,
encoder_context, seq_bits_per_second);
- return intel_encoder_check_brc_vp9_sequence_parameter(ctx,
encode_state, encoder_context);
+ return intel_encoder_check_brc_vp9_sequence_parameter(ctx,
encode_state, encoder_context, seq_bits_per_second);
// TODO: other codecs
@@ -545,7 +546,8 @@
intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,
static void
intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
struct
intel_encoder_context *encoder_context,
- VAEncMiscParameterRateC
ontrol *misc)
+ VAEncMiscParameterRateC
ontrol *misc,
+ int
*hl_bitrate_updated)
{
int temporal_id = 0;
@@ -577,6 +579,9 @@
intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
encoder_context->brc.window_size = misc->window_size;
encoder_context->brc.need_reset = 1;
}
+
+ if (temporal_id == encoder_context->layer.num_layers - 1)
+ *hl_bitrate_updated = 1;
}
static void
@@ -651,11 +656,13 @@
intel_encoder_check_brc_parameter(VADriverContextP ctx,
VAStatus ret;
VAEncMiscParameterBuffer *misc_param;
int i, j;
+ int hl_bitrate_updated = 0; // Indicate whether the bitrate
for the highest level is changed in misc parameters
+ unsigned int seq_bits_per_second = 0;
if (!(encoder_context->rate_control_mode& (VA_RC_CBR |
VA_RC_VBR)))
return VA_STATUS_SUCCESS;
- ret = intel_encoder_check_brc_sequence_parameter(ctx,
encode_state, encoder_context);
+ ret = intel_encoder_check_brc_sequence_parameter(ctx,
encode_state, encoder_context,&seq_bits_per_second);
if (ret)
return ret;
@@ -677,7 +684,8 @@
intel_encoder_check_brc_parameter(VADriverContextP ctx,
intel_encoder_check_rate_control_parameter(ctx,
encode
r_context,
- (VAEncM
iscParameterRateControl *)misc_param->data);
+ (VAEncM
iscParameterRateControl *)misc_param->data,
+&hl_bitrate_updated);
break;
@@ -698,6 +706,14 @@
intel_encoder_check_brc_parameter(VADriverContextP ctx,
}
}
+ if (!hl_bitrate_updated&& seq_bits_per_second&&
+ encoder_context->brc.bits_per_second[encoder_context-
Post by Xiang, Haihao
layer.num_layers - 1] != seq_bits_per_second) {
+
+ encoder_context->brc.bits_per_second[encoder_context-
Post by Xiang, Haihao
layer.num_layers - 1] = seq_bits_per_second;
+ encoder_context->brc.need_reset = 1;
+
+ }
+
return VA_STATUS_SUCCESS;
}
Mark Thompson
2016-12-29 18:52:43 UTC
Permalink
Post by Zhao Yakui
Post by Xiang, Haihao
Post by Zhao Yakui
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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_context->brc.hrd_initial_buffer_fullness) {
encoder_context->brc.hrd_buffer_size = seq_param->bits_per_second << 1;
@@ -414,9 +418,13 @@ intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}

- if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop ||
- bits_per_second != encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
+ if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop) {
encoder_context->brc.num_pframes_in_gop = num_pframes_in_gop;
+ 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;
}
@@ -481,7 +489,8 @@ intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}

- if (encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second) {
+ if (encode_state->frame_has_sequence_parameters && !encode_state->frame_has_rc_parameters &&
+ encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second) {
encoder_context->brc.bits_per_second[0] = seq_param->bits_per_second;
encoder_context->brc.need_reset = 1;
}
@@ -507,13 +516,17 @@ intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP ctx,
else
gop_size = seq_param->intra_period;

- if (encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second ||
- encoder_context->brc.gop_size != gop_size) {
- encoder_context->brc.bits_per_second[0] = seq_param->bits_per_second;
+ if (encoder_context->brc.gop_size != gop_size) {
encoder_context->brc.gop_size = gop_size;
encoder_context->brc.need_reset = 1;
}

+ if (encode_state->frame_has_sequence_parameters && !encode_state->frame_has_rc_parameters &&
+ encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second) {
+ encoder_context->brc.bits_per_second[0] = seq_param->bits_per_second;
+ encoder_context->brc.need_reset = 1;
+ }
+
return VA_STATUS_SUCCESS;
}

@@ -544,6 +557,7 @@ intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,

static void
intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
+ struct encode_state *encode_state,
struct intel_encoder_context *encoder_context,
VAEncMiscParameterRateControl *misc)
{
@@ -558,7 +572,8 @@ intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
if (misc->rc_flags.bits.reset)
encoder_context->brc.need_reset = 1;

- if (encoder_context->brc.bits_per_second[temporal_id] != misc->bits_per_second) {
+ if (encode_state->frame_has_rc_parameters &&
+ encoder_context->brc.bits_per_second[temporal_id] != misc->bits_per_second) {
encoder_context->brc.bits_per_second[temporal_id] = misc->bits_per_second;
encoder_context->brc.need_reset = 1;
}
@@ -676,7 +691,7 @@ intel_encoder_check_brc_parameter(VADriverContextP ctx,

case VAEncMiscParameterTypeRateControl:
intel_encoder_check_rate_control_parameter(ctx,
- encoder_context,
+ encode_state, encoder_context,
(VAEncMiscParameterRateControl *)misc_param->data);
break;

@@ -1299,6 +1314,9 @@ intel_encoder_end_picture(VADriverContextP ctx,
*/
encoder_context->brc.num_roi = 0;

+ encode_state->frame_has_sequence_parameters = 0;
+ encode_state->frame_has_rc_parameters = 0;
+
return VA_STATUS_SUCCESS;
}
Zhao Yakui
2016-12-30 00:14:33 UTC
Permalink
Post by Mark Thompson
Post by Zhao Yakui
Post by Xiang, Haihao
Post by Zhao Yakui
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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).
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?
Post by Mark Thompson
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,
vaStatus = I965_RENDER_ENCODE_BUFFER(sequence_parameter_ext);
+ encode->frame_has_sequence_parameters = 1;
break;
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_context->brc.hrd_initial_buffer_fullness) {
encoder_context->brc.hrd_buffer_size = seq_param->bits_per_second<< 1;
@@ -414,9 +418,13 @@ intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}
- if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop ||
- bits_per_second != encoder_context->brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
+ if (num_pframes_in_gop != encoder_context->brc.num_pframes_in_gop) {
encoder_context->brc.num_pframes_in_gop = num_pframes_in_gop;
+ 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;
}
@@ -481,7 +489,8 @@ intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
encoder_context->brc.need_reset = 1;
}
- if (encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second) {
+ if (encode_state->frame_has_sequence_parameters&& !encode_state->frame_has_rc_parameters&&
+ encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second) {
encoder_context->brc.bits_per_second[0] = seq_param->bits_per_second;
encoder_context->brc.need_reset = 1;
}
@@ -507,13 +516,17 @@ intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP ctx,
else
gop_size = seq_param->intra_period;
- if (encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second ||
- encoder_context->brc.gop_size != gop_size) {
- encoder_context->brc.bits_per_second[0] = seq_param->bits_per_second;
+ if (encoder_context->brc.gop_size != gop_size) {
encoder_context->brc.gop_size = gop_size;
encoder_context->brc.need_reset = 1;
}
+ if (encode_state->frame_has_sequence_parameters&& !encode_state->frame_has_rc_parameters&&
+ encoder_context->brc.bits_per_second[0] != seq_param->bits_per_second) {
+ encoder_context->brc.bits_per_second[0] = seq_param->bits_per_second;
+ encoder_context->brc.need_reset = 1;
+ }
+
return VA_STATUS_SUCCESS;
}
@@ -544,6 +557,7 @@ intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,
static void
intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
+ struct encode_state *encode_state,
struct intel_encoder_context *encoder_context,
VAEncMiscParameterRateControl *misc)
{
@@ -558,7 +572,8 @@ intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
if (misc->rc_flags.bits.reset)
encoder_context->brc.need_reset = 1;
- if (encoder_context->brc.bits_per_second[temporal_id] != misc->bits_per_second) {
+ if (encode_state->frame_has_rc_parameters&&
+ encoder_context->brc.bits_per_second[temporal_id] != misc->bits_per_second) {
encoder_context->brc.bits_per_second[temporal_id] = misc->bits_per_second;
encoder_context->brc.need_reset = 1;
}
@@ -676,7 +691,7 @@ intel_encoder_check_brc_parameter(VADriverContextP ctx,
intel_encoder_check_rate_control_parameter(ctx,
- encoder_context,
+ encode_state, encoder_context,
(VAEncMiscParameterRateControl *)misc_param->data);
break;
@@ -1299,6 +1314,9 @@ intel_encoder_end_picture(VADriverContextP ctx,
*/
encoder_context->brc.num_roi = 0;
+ encode_state->frame_has_sequence_parameters = 0;
+ encode_state->frame_has_rc_parameters = 0;
+
return VA_STATUS_SUCCESS;
}
_______________________________________________
Libva mailing list
https://lists.freedesktop.org/mailman/listinfo/libva
Mark Thompson
2016-12-30 17:54:38 UTC
Permalink
Post by Zhao Yakui
Post by Mark Thompson
Post by Zhao Yakui
Post by Xiang, Haihao
Post by Zhao Yakui
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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).
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.
Hmm, yes. That would entail discarding the contents of encoder_context->misc_param[VAEncMiscParameterTypeRateControl] when new sequence parameters are provided? (In the same way that ROI parameters are discarded after every frame.) I think that's a much nicer answer than what I had, and is more consistent as well.

Thanks,

- Mark
Zhao Yakui
2017-01-03 01:50:55 UTC
Permalink
Post by Mark Thompson
Post by Zhao Yakui
Post by Mark Thompson
Post by Zhao Yakui
Post by Xiang, Haihao
Post by Zhao Yakui
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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).
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.
Hmm, yes. That would entail discarding the contents of encoder_context->misc_param[VAEncMiscParameterTypeRateControl] when new sequence parameters are provided? (In the same way that ROI parameters are discarded after every frame.) I think that's a much nicer answer than what I had, and is more consistent as well.
Yes. It need to discard the content of
encoder_context->misc_param[VAEncMiscParameterTypeRateControl] when the
new sequence is provided.
Unfortunately the current code doesn't discard it.

Will you please send out the patch that discards the
encoder_context->misc_param[VAEncMiscParameterTypeRateControl]?

Thanks
Yakui
Post by Mark Thompson
Thanks,
- Mark
Xiang, Haihao
2017-01-03 06:23:20 UTC
Permalink
Post by Zhao Yakui
Post by Zhao Yakui
Post by Zhao Yakui
Post by Xiang, Haihao
Post by Zhao Yakui
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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).
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.
Hmm, yes.  That would entail discarding the contents of
encoder_context->misc_param[VAEncMiscParameterTypeRateControl] when
new sequence parameters are provided?  (In the same way that ROI
parameters are discarded after every frame.)  I think that's a much
nicer answer than what I had, and is more consistent as well.
Yes. It need to discard the content of
encoder_context->misc_param[VAEncMiscParameterTypeRateControl] when the
new sequence is provided.
Unfortunately the current code doesn't discard it.
That is because we didn't save the misc parameters in the common code
 before and we use some misc parameters for next frames,
hence intel_encoder_context[...]
Post by Zhao Yakui
Will you please send out the patch that discards the
encoder_context->misc_param[VAEncMiscParameterTypeRateControl]?
Currently only gen6_mfc_common.c doesn't use generic ROI parameters
in intel_encoder_context. I am cleaning up gen6_mfc_common.c so that we
can clear all misc parameters in the same way.

Note VAEncMiscParameterTypeVP9PerSegmantParam is not a misc parameter
indeed :(
Post by Zhao Yakui
Thanks
    Yakui
Thanks,
- Mark
_______________________________________________
Libva mailing list
https://lists.freedesktop.org/mailman/listinfo/libva
Xiang, Haihao
2016-12-30 00:51:48 UTC
Permalink
Post by Zhao Yakui
Post by Xiang, Haihao
Post by Zhao Yakui
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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).
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,
 
             vaStatus =
I965_RENDER_ENCODE_BUFFER(sequence_parameter_ext);
+            encode->frame_has_sequence_parameters = 1;
             break;
 
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-
Post by Zhao Yakui
brc.num_pframes_in_gop ||
         num_bframes_in_gop != encoder_context-
Post by Zhao Yakui
brc.num_bframes_in_gop ||
-        bits_per_second != encoder_context-
Post by Zhao Yakui
brc.bits_per_second[encoder_context->layer.num_layers - 1] ||
         framerate.num != encoder_context-
Post by Zhao Yakui
brc.framerate[encoder_context->layer.num_layers - 1].num ||
         framerate.den != encoder_context-
Post by Zhao Yakui
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-
Post by Zhao Yakui
layer.num_layers - 1] = bits_per_second;
         encoder_context->brc.framerate[encoder_context-
Post by Zhao Yakui
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-
Post by Zhao Yakui
brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
+        encoder_context->brc.bits_per_second[encoder_context-
Post by Zhao Yakui
layer.num_layers - 1] = bits_per_second;
+        encoder_context->brc.need_reset = 1;
+    }
+
     if (!encoder_context->brc.hrd_buffer_size ||
         !encoder_context->brc.hrd_initial_buffer_fullness) {
         encoder_context->brc.hrd_buffer_size = seq_param-
Post by Zhao Yakui
bits_per_second << 1;
@@ -414,9 +418,13 @@
intel_encoder_check_brc_vp8_sequence_parameter(VADriverContextP ctx,
         encoder_context->brc.need_reset = 1;
     }
 
-    if (num_pframes_in_gop != encoder_context-
Post by Zhao Yakui
brc.num_pframes_in_gop ||
-        bits_per_second != encoder_context-
Post by Zhao Yakui
brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
+    if (num_pframes_in_gop != encoder_context-
Post by Zhao Yakui
brc.num_pframes_in_gop) {
         encoder_context->brc.num_pframes_in_gop =
num_pframes_in_gop;
+        encoder_context->brc.need_reset = 1;
+    }
+
+    if (encode_state->frame_has_sequence_parameters &&
!encode_state->frame_has_rc_parameters &&
+        bits_per_second != encoder_context-
Post by Zhao Yakui
brc.bits_per_second[encoder_context->layer.num_layers - 1]) {
         encoder_context->brc.bits_per_second[encoder_context-
Post by Zhao Yakui
layer.num_layers - 1] = bits_per_second;
         encoder_context->brc.need_reset = 1;
     }
@@ -481,7 +489,8 @@
intel_encoder_check_brc_hevc_sequence_parameter(VADriverContextP ctx,
         encoder_context->brc.need_reset = 1;
     }
 
-    if (encoder_context->brc.bits_per_second[0] != seq_param-
Post by Zhao Yakui
bits_per_second) {
+    if (encode_state->frame_has_sequence_parameters &&
!encode_state->frame_has_rc_parameters &&
+        encoder_context->brc.bits_per_second[0] != seq_param-
Post by Zhao Yakui
bits_per_second) {
         encoder_context->brc.bits_per_second[0] = seq_param-
Post by Zhao Yakui
bits_per_second;
         encoder_context->brc.need_reset = 1;
     }
@@ -507,13 +516,17 @@
intel_encoder_check_brc_vp9_sequence_parameter(VADriverContextP ctx,
     else
         gop_size = seq_param->intra_period;
 
-    if (encoder_context->brc.bits_per_second[0] != seq_param-
Post by Zhao Yakui
bits_per_second ||
-        encoder_context->brc.gop_size != gop_size) {
-        encoder_context->brc.bits_per_second[0] = seq_param-
Post by Zhao Yakui
bits_per_second;
+    if (encoder_context->brc.gop_size != gop_size) {
         encoder_context->brc.gop_size = gop_size;
         encoder_context->brc.need_reset = 1;
     }
 
+    if (encode_state->frame_has_sequence_parameters &&
!encode_state->frame_has_rc_parameters &&
+        encoder_context->brc.bits_per_second[0] != seq_param-
Post by Zhao Yakui
bits_per_second) {
+        encoder_context->brc.bits_per_second[0] = seq_param-
Post by Zhao Yakui
bits_per_second;
+        encoder_context->brc.need_reset = 1;
+    }
+
     return VA_STATUS_SUCCESS;
 }
 
@@ -544,6 +557,7 @@
intel_encoder_check_brc_sequence_parameter(VADriverContextP ctx,
 
 static void
 intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
+                                           struct encode_state
*encode_state,
                                            struct
intel_encoder_context *encoder_context,
                                            VAEncMiscParameterRateCon
trol *misc)
 {
@@ -558,7 +572,8 @@
intel_encoder_check_rate_control_parameter(VADriverContextP ctx,
     if (misc->rc_flags.bits.reset)
         encoder_context->brc.need_reset = 1;
 
-    if (encoder_context->brc.bits_per_second[temporal_id] != misc-
Post by Zhao Yakui
bits_per_second) {
+    if (encode_state->frame_has_rc_parameters &&
+        encoder_context->brc.bits_per_second[temporal_id] != misc-
Post by Zhao Yakui
bits_per_second) {
         encoder_context->brc.bits_per_second[temporal_id] = misc-
Post by Zhao Yakui
bits_per_second;
         encoder_context->brc.need_reset = 1;
     }
@@ -676,7 +691,7 @@
intel_encoder_check_brc_parameter(VADriverContextP ctx,
 
                 intel_encoder_check_rate_control_parameter(ctx,
-                                                           encoder_c
ontext,
+                                                           encode_st
ate, encoder_context,
                                                            (VAEncMis
cParameterRateControl *)misc_param->data);
                 break;
 
@@ -1299,6 +1314,9 @@ intel_encoder_end_picture(VADriverContextP ctx,
      */
     encoder_context->brc.num_roi = 0;
 
+    encode_state->frame_has_sequence_parameters = 0;
+    encode_state->frame_has_rc_parameters = 0;
+
     return VA_STATUS_SUCCESS;
 }
 
_______________________________________________
Libva mailing list
https://lists.freedesktop.org/mailman/listinfo/libva
Mark Thompson
2016-12-30 17:44:55 UTC
Permalink
Post by Xiang, Haihao
Post by Mark Thompson
Post by Zhao Yakui
Post by Xiang, Haihao
Post by Zhao Yakui
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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).
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.
This is not the case - in the absence of a new RC parameter structure, the one that was set in the middle of the first sequence continues to apply forever (encoder_context->misc_param[] is never cleared), so hl_bitrate_updated is always 1 after that point.

To show that more concretely, with this patch applied to print the internal state:

diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index 0a648d4..a7c92ce 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -718,6 +718,12 @@ intel_encoder_check_brc_parameter(VADriverContextP ctx,

}

+ printf("hl_bitrate_updated = %d, is_new_sequence = %d, need_reset = %d, "
+ "seq_bits_per_second = %d, brc bits_per_second = %d\n",
+ hl_bitrate_updated, encoder_context->is_new_sequence,
+ encoder_context->brc.need_reset, seq_bits_per_second,
+ encoder_context->brc.bits_per_second[0]);
+
return VA_STATUS_SUCCESS;
}


I then encode with a GOP size of 2 and and bitrate increasing by 1M on each IDR frame, providing RC parameters only with the first:

hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 1, seq_bits_per_second = 1000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 2000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 3000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 4000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 5000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 6000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 7000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 8000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 9000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0, seq_bits_per_second = 10000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0, seq_bits_per_second = 0, brc bits_per_second = 1000000


Thanks,

- Mark
Xiang, Haihao
2017-01-03 06:42:52 UTC
Permalink
Post by Mark Thompson
Post by Xiang, Haihao
Post by Zhao Yakui
Post by Xiang, Haihao
Post by Zhao Yakui
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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).
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.
This is not the case - in the absence of a new RC parameter
structure, the one that was set in the middle of the first sequence
continues to apply forever (encoder_context->misc_param[] is never
cleared), so hl_bitrate_updated is always 1 after that point.
You are right, I forgot encoder_context->misc_param[]
(except VAEncMiscParameterTypeROI) is not cleared presently. It would
be better we use the same way for all misc parameters, I will file a
patch to clear encoder_context->misc_param[]. 
Post by Mark Thompson
diff --git a/src/i965_encoder.c b/src/i965_encoder.c
index 0a648d4..a7c92ce 100644
--- a/src/i965_encoder.c
+++ b/src/i965_encoder.c
@@ -718,6 +718,12 @@
intel_encoder_check_brc_parameter(VADriverContextP ctx,
 
     }
 
+    printf("hl_bitrate_updated = %d, is_new_sequence = %d,
need_reset = %d, "
+           "seq_bits_per_second = %d, brc bits_per_second = %d\n",
+           hl_bitrate_updated, encoder_context->is_new_sequence,
+           encoder_context->brc.need_reset, seq_bits_per_second,
+           encoder_context->brc.bits_per_second[0]);
+
     return VA_STATUS_SUCCESS;
 }
Thanks for your patch to demonstrate that problem. I should check the
code more carefully.
Post by Mark Thompson
I then encode with a GOP size of 2 and and bitrate increasing by 1M
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 1,
seq_bits_per_second = 1000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
seq_bits_per_second = 2000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
seq_bits_per_second = 3000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
seq_bits_per_second = 4000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
seq_bits_per_second = 5000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
seq_bits_per_second = 6000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
seq_bits_per_second = 7000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
seq_bits_per_second = 8000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
seq_bits_per_second = 9000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
seq_bits_per_second = 0, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 1, need_reset = 0,
seq_bits_per_second = 10000000, brc bits_per_second = 1000000
hl_bitrate_updated = 1, is_new_sequence = 0, need_reset = 0,
seq_bits_per_second = 0, brc bits_per_second = 1000000
Thanks,
- Mark
Mark Thompson
2017-01-04 00:07:22 UTC
Permalink
Post by Mark Thompson
Post by Xiang, Haihao
Post by Mark Thompson
Post by Zhao Yakui
Post by Xiang, Haihao
Post by Zhao Yakui
Post by Xiang, Haihao
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.
Post by Xiang, Haihao
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).
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.
This is not the case - in the absence of a new RC parameter
structure, the one that was set in the middle of the first sequence
continues to apply forever (encoder_context->misc_param[] is never
cleared), so hl_bitrate_updated is always 1 after that point.
You are right, I forgot encoder_context->misc_param[]
(except VAEncMiscParameterTypeROI) is not cleared presently. It would
be better we use the same way for all misc parameters, I will file a
patch to clear encoder_context->misc_param[].
Ok, sounds good :)

Thanks,

- Mark

Loading...