[Libva] [PATCH v5 intel-driver 11/11] decoder: h264: fix frame store logic for MVC.
Zhao, Yakui
yakui.zhao at intel.com
Mon Jun 9 01:52:12 PDT 2014
On Fri, 2014-06-06 at 03:15 -0600, Gwenole Beauchesne wrote:
> In strict MVC decoding mode, when only the necessary set of inter-view
> reference pictures are passed to the ReferenceFrames array for decoding
> the current picture, we should not re-use a frame store id that might
> be needed for decoding another view component in the same access unit.
>
> One way to solve this problem is to track when the VA surface in a
> specified frame store id was last referenced. So, a "ref_age" field
> is introduced to the GenFrameStore struct and is updated whenever
> the surface is being referenced.
>
> Additionally, the list of retired refs candidates (free_refs) is kept
> ordered by increasing ref_age. That way, we can immediately know what
> is the oldest frame store id to recycle.
>
> Let deltaAge = CurrAge - RefAge:
> If deltaAge > 1, we know for sure that the VA surface is gone ;
> If deltaAge = 1, the surface could be re-used for inter prediction ;
> If deltaAge = 0, the surface could be re-used for inter-view prediction.
>
> The ref_age in each Frame Store entry is always current, i.e. it is
> the same for all reference frames that intervened in the decoding
> process of all inter view components of the previous access unit. The
> age tracks access units.
This patch looks good to me. IMO it can be pushed to the staging branch.
Before it is cherry-picked to the master branch, it will be better that
more decoding bitstreams are tested to avoid the possible regression of
H264 decoding on Ivy/Sandybridge.
Thanks.
Yakui
>
> Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne at intel.com>
> ---
> src/gen6_mfd.c | 4 +-
> src/gen6_mfd.h | 1 +
> src/gen75_mfd.c | 1 +
> src/gen7_mfd.c | 4 +-
> src/gen7_mfd.h | 1 +
> src/gen8_mfd.c | 6 +--
> src/i965_avc_bsd.c | 4 +-
> src/i965_decoder.h | 15 ++++++
> src/i965_decoder_utils.c | 114 +++++++++++++++++++++++++++++++---------------
> src/i965_decoder_utils.h | 11 +++--
> src/i965_media_h264.h | 1 +
> src/intel_media.h | 1 +
> 12 files changed, 115 insertions(+), 48 deletions(-)
>
> diff --git a/src/gen6_mfd.c b/src/gen6_mfd.c
> index 437ad3b..8128a80 100755
> --- a/src/gen6_mfd.c
> +++ b/src/gen6_mfd.c
> @@ -61,6 +61,7 @@ gen6_mfd_init_avc_surface(VADriverContextP ctx,
>
> if (!gen6_avc_surface) {
> gen6_avc_surface = calloc(sizeof(GenAvcSurface), 1);
> + gen6_avc_surface->frame_store_id = -1;
> assert((obj_surface->size & 0x3f) == 0);
> obj_surface->private_data = gen6_avc_surface;
> }
> @@ -825,7 +826,8 @@ gen6_mfd_avc_decode_init(VADriverContextP ctx,
>
> assert(decode_state->pic_param && decode_state->pic_param->buffer);
> pic_param = (VAPictureParameterBufferH264 *)decode_state->pic_param->buffer;
> - intel_update_avc_frame_store_index(ctx, decode_state, pic_param, gen6_mfd_context->reference_surface);
> + intel_update_avc_frame_store_index(ctx, decode_state, pic_param,
> + gen6_mfd_context->reference_surface, &gen6_mfd_context->fs_ctx);
> width_in_mbs = ((pic_param->picture_width_in_mbs_minus1 + 1) & 0xff);
>
> /* Current decoded picture */
> diff --git a/src/gen6_mfd.h b/src/gen6_mfd.h
> index de131d6..f499803 100644
> --- a/src/gen6_mfd.h
> +++ b/src/gen6_mfd.h
> @@ -62,6 +62,7 @@ struct gen6_mfd_context
> VAIQMatrixBufferMPEG2 mpeg2;
> } iq_matrix;
>
> + GenFrameStoreContext fs_ctx;
> GenFrameStore reference_surface[MAX_GEN_REFERENCE_FRAMES];
> GenBuffer post_deblocking_output;
> GenBuffer pre_deblocking_output;
> diff --git a/src/gen75_mfd.c b/src/gen75_mfd.c
> index 67d4c51..1a068ec 100644
> --- a/src/gen75_mfd.c
> +++ b/src/gen75_mfd.c
> @@ -67,6 +67,7 @@ gen75_mfd_init_avc_surface(VADriverContextP ctx,
>
> if (!gen7_avc_surface) {
> gen7_avc_surface = calloc(sizeof(GenAvcSurface), 1);
> + gen7_avc_surface->frame_store_id = -1;
> assert((obj_surface->size & 0x3f) == 0);
> obj_surface->private_data = gen7_avc_surface;
> }
> diff --git a/src/gen7_mfd.c b/src/gen7_mfd.c
> index 4141b83..07466b1 100755
> --- a/src/gen7_mfd.c
> +++ b/src/gen7_mfd.c
> @@ -65,6 +65,7 @@ gen7_mfd_init_avc_surface(VADriverContextP ctx,
>
> if (!gen7_avc_surface) {
> gen7_avc_surface = calloc(sizeof(GenAvcSurface), 1);
> + gen7_avc_surface->frame_store_id = -1;
> assert((obj_surface->size & 0x3f) == 0);
> obj_surface->private_data = gen7_avc_surface;
> }
> @@ -740,7 +741,8 @@ gen7_mfd_avc_decode_init(VADriverContextP ctx,
>
> assert(decode_state->pic_param && decode_state->pic_param->buffer);
> pic_param = (VAPictureParameterBufferH264 *)decode_state->pic_param->buffer;
> - intel_update_avc_frame_store_index(ctx, decode_state, pic_param, gen7_mfd_context->reference_surface);
> + intel_update_avc_frame_store_index(ctx, decode_state, pic_param,
> + gen7_mfd_context->reference_surface, &gen7_mfd_context->fs_ctx);
> width_in_mbs = pic_param->picture_width_in_mbs_minus1 + 1;
> height_in_mbs = pic_param->picture_height_in_mbs_minus1 + 1;
> assert(width_in_mbs > 0 && width_in_mbs <= 256); /* 4K */
> diff --git a/src/gen7_mfd.h b/src/gen7_mfd.h
> index 0200216..af8e960 100644
> --- a/src/gen7_mfd.h
> +++ b/src/gen7_mfd.h
> @@ -77,6 +77,7 @@ struct gen7_mfd_context
> VAIQMatrixBufferH264 h264; /* flat scaling lists (default) */
> } iq_matrix;
>
> + GenFrameStoreContext fs_ctx;
> GenFrameStore reference_surface[MAX_GEN_REFERENCE_FRAMES];
> GenBuffer post_deblocking_output;
> GenBuffer pre_deblocking_output;
> diff --git a/src/gen8_mfd.c b/src/gen8_mfd.c
> index 065302d..1a3d063 100644
> --- a/src/gen8_mfd.c
> +++ b/src/gen8_mfd.c
> @@ -27,10 +27,7 @@
> *
> */
>
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -#include <assert.h>
> +#include "sysdeps.h"
> #include <math.h>
> #include <va/va_dec_jpeg.h>
> #include <va/va_dec_vp8.h>
> @@ -74,6 +71,7 @@ gen8_mfd_init_avc_surface(VADriverContextP ctx,
>
> if (!gen7_avc_surface) {
> gen7_avc_surface = calloc(sizeof(GenAvcSurface), 1);
> + gen7_avc_surface->frame_store_id = -1;
> assert((obj_surface->size & 0x3f) == 0);
> obj_surface->private_data = gen7_avc_surface;
> }
> diff --git a/src/i965_avc_bsd.c b/src/i965_avc_bsd.c
> index aca3c01..ebeb2a6 100644
> --- a/src/i965_avc_bsd.c
> +++ b/src/i965_avc_bsd.c
> @@ -51,6 +51,7 @@ i965_avc_bsd_init_avc_bsd_surface(VADriverContextP ctx,
>
> if (!avc_bsd_surface) {
> avc_bsd_surface = calloc(sizeof(GenAvcSurface), 1);
> + avc_bsd_surface->frame_store_id = -1;
> assert((obj_surface->size & 0x3f) == 0);
> obj_surface->private_data = avc_bsd_surface;
> }
> @@ -795,7 +796,8 @@ i965_avc_bsd_pipeline(VADriverContextP ctx, struct decode_state *decode_state, v
>
> assert(decode_state->pic_param && decode_state->pic_param->buffer);
> pic_param = (VAPictureParameterBufferH264 *)decode_state->pic_param->buffer;
> - intel_update_avc_frame_store_index(ctx, decode_state, pic_param, i965_h264_context->fsid_list);
> + intel_update_avc_frame_store_index(ctx, decode_state, pic_param,
> + i965_h264_context->fsid_list, &i965_h264_context->fs_ctx);
>
> i965_h264_context->enable_avc_ildb = 0;
> i965_h264_context->picture.i_flag = 1;
> diff --git a/src/i965_decoder.h b/src/i965_decoder.h
> index 01c093f..14d4d0c 100644
> --- a/src/i965_decoder.h
> +++ b/src/i965_decoder.h
> @@ -39,6 +39,21 @@ struct gen_frame_store {
> VASurfaceID surface_id;
> int frame_store_id;
> struct object_surface *obj_surface;
> +
> + /* This represents the time when this frame store was last used to
> + hold a reference frame. This is not connected to a presentation
> + timestamp (PTS), and this is not a common decoding time stamp
> + (DTS) either. It serves the purpose of tracking retired
> + reference frame candidates.
> +
> + This is only used for H.264 decoding on platforms before Haswell */
> + uint64_t ref_age;
> +};
> +
> +typedef struct gen_frame_store_context GenFrameStoreContext;
> +struct gen_frame_store_context {
> + uint64_t age;
> + int prev_poc;
> };
>
> typedef struct gen_buffer GenBuffer;
> diff --git a/src/i965_decoder_utils.c b/src/i965_decoder_utils.c
> index 7833919..64be7b6 100644
> --- a/src/i965_decoder_utils.c
> +++ b/src/i965_decoder_utils.c
> @@ -22,10 +22,11 @@
> */
>
> #include "sysdeps.h"
> -
> +#include <limits.h>
> #include <alloca.h>
>
> #include "intel_batchbuffer.h"
> +#include "intel_media.h"
> #include "i965_drv_video.h"
> #include "i965_decoder_utils.h"
> #include "i965_defines.h"
> @@ -254,6 +255,21 @@ avc_gen_default_iq_matrix(VAIQMatrixBufferH264 *iq_matrix)
> memset(&iq_matrix->ScalingList8x8, 16, sizeof(iq_matrix->ScalingList8x8));
> }
>
> +/* Returns the POC of the supplied VA picture */
> +static int
> +avc_get_picture_poc(const VAPictureH264 *va_pic)
> +{
> + int structure, field_poc[2];
> +
> + structure = va_pic->flags &
> + (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD);
> + field_poc[0] = structure != VA_PICTURE_H264_BOTTOM_FIELD ?
> + va_pic->TopFieldOrderCnt : INT_MAX;
> + field_poc[1] = structure != VA_PICTURE_H264_TOP_FIELD ?
> + va_pic->BottomFieldOrderCnt : INT_MAX;
> + return MIN(field_poc[0], field_poc[1]);
> +}
> +
> /* Returns a unique picture ID that represents the supplied VA surface object */
> int
> avc_get_picture_id(struct object_surface *obj_surface)
> @@ -471,68 +487,92 @@ gen6_send_avc_ref_idx_state(
> );
> }
>
> +/* Comparison function for sorting out the array of free frame store entries */
> +static int
> +compare_avc_ref_store_func(const void *p1, const void *p2)
> +{
> + const GenFrameStore * const fs1 = *((GenFrameStore **)p1);
> + const GenFrameStore * const fs2 = *((GenFrameStore **)p2);
> +
> + return fs1->ref_age - fs2->ref_age;
> +}
> +
> void
> intel_update_avc_frame_store_index(
> VADriverContextP ctx,
> struct decode_state *decode_state,
> VAPictureParameterBufferH264 *pic_param,
> - GenFrameStore frame_store[MAX_GEN_REFERENCE_FRAMES]
> + GenFrameStore frame_store[MAX_GEN_REFERENCE_FRAMES],
> + GenFrameStoreContext *fs_ctx
> )
> {
> GenFrameStore *free_refs[MAX_GEN_REFERENCE_FRAMES];
> - int i, j, n, num_free_refs;
> + uint32_t used_refs = 0, add_refs = 0;
> + uint64_t age;
> + int i, n, num_free_refs;
> +
> + /* Detect changes of access unit */
> + const int poc = avc_get_picture_poc(&pic_param->CurrPic);
> + if (fs_ctx->age == 0)
> + fs_ctx->age++;
> + else if (fs_ctx->prev_poc != poc) {
> + fs_ctx->prev_poc = poc;
> + fs_ctx->age++;
> + }
> + fs_ctx->prev_poc = poc;
> + age = fs_ctx->age;
>
> - /* Remove obsolete entries from the internal DPB */
> - for (i = 0, n = 0; i < MAX_GEN_REFERENCE_FRAMES; i++) {
> - GenFrameStore * const fs = &frame_store[i];
> - if (fs->surface_id == VA_INVALID_ID || !fs->obj_surface) {
> - free_refs[n++] = fs;
> + /* Tag entries that are still available in our Frame Store */
> + for (i = 0; i < ARRAY_ELEMS(decode_state->reference_objects); i++) {
> + struct object_surface * const obj_surface =
> + decode_state->reference_objects[i];
> + if (!obj_surface)
> continue;
> - }
>
> - // Find whether the current entry is still a valid reference frame
> - for (j = 0; j < ARRAY_ELEMS(decode_state->reference_objects); j++) {
> - struct object_surface * const obj_surface =
> - decode_state->reference_objects[j];
> - if (obj_surface && obj_surface == fs->obj_surface)
> - break;
> + GenAvcSurface * const avc_surface = obj_surface->private_data;
> + if (avc_surface->frame_store_id >= 0) {
> + GenFrameStore * const fs =
> + &frame_store[avc_surface->frame_store_id];
> + if (fs->surface_id == obj_surface->base.id) {
> + fs->obj_surface = obj_surface;
> + fs->ref_age = age;
> + used_refs |= 1 << fs->frame_store_id;
> + continue;
> + }
> }
> + add_refs |= 1 << i;
> + }
>
> - // ... or remove it
> - if (j == ARRAY_ELEMS(decode_state->reference_objects)) {
> - fs->surface_id = VA_INVALID_ID;
> + /* Build and sort out the list of retired candidates. The resulting
> + list is ordered by increasing age when they were last used */
> + for (i = 0, n = 0; i < MAX_GEN_REFERENCE_FRAMES; i++) {
> + if (!(used_refs & (1 << i))) {
> + GenFrameStore * const fs = &frame_store[i];
> fs->obj_surface = NULL;
> - fs->frame_store_id = -1;
> free_refs[n++] = fs;
> }
> }
> num_free_refs = n;
> + qsort(&free_refs[0], n, sizeof(free_refs[0]), compare_avc_ref_store_func);
>
> /* Append the new reference frames */
> for (i = 0, n = 0; i < ARRAY_ELEMS(decode_state->reference_objects); i++) {
> struct object_surface * const obj_surface =
> decode_state->reference_objects[i];
> - if (!obj_surface)
> + if (!obj_surface || !(add_refs & (1 << i)))
> continue;
>
> - // Find whether the current frame is not already in our frame store
> - for (j = 0; j < MAX_GEN_REFERENCE_FRAMES; j++) {
> - GenFrameStore * const fs = &frame_store[j];
> - if (fs->obj_surface == obj_surface)
> - break;
> - }
> -
> - // ... or add it
> - if (j == MAX_GEN_REFERENCE_FRAMES) {
> - if (n < num_free_refs) {
> - GenFrameStore * const fs = free_refs[n++];
> - fs->surface_id = obj_surface->base.id;
> - fs->obj_surface = obj_surface;
> - fs->frame_store_id = fs - frame_store;
> - continue;
> - }
> - WARN_ONCE("No free slot found for DPB reference list!!!\n");
> + GenAvcSurface * const avc_surface = obj_surface->private_data;
> + if (n < num_free_refs) {
> + GenFrameStore * const fs = free_refs[n++];
> + fs->surface_id = obj_surface->base.id;
> + fs->obj_surface = obj_surface;
> + fs->frame_store_id = fs - frame_store;
> + fs->ref_age = age;
> + avc_surface->frame_store_id = fs->frame_store_id;
> + continue;
> }
> + WARN_ONCE("No free slot found for DPB reference list!!!\n");
> }
> }
>
> diff --git a/src/i965_decoder_utils.h b/src/i965_decoder_utils.h
> index 0ffbd7f..3d39b21 100644
> --- a/src/i965_decoder_utils.h
> +++ b/src/i965_decoder_utils.h
> @@ -95,10 +95,13 @@ intel_decoder_sanity_check_input(VADriverContextP ctx,
> struct decode_state *decode_state);
>
> void
> -intel_update_avc_frame_store_index(VADriverContextP ctx,
> - struct decode_state *decode_state,
> - VAPictureParameterBufferH264 *pic_param,
> - GenFrameStore frame_store[MAX_GEN_REFERENCE_FRAMES]);
> +intel_update_avc_frame_store_index(
> + VADriverContextP ctx,
> + struct decode_state *decode_state,
> + VAPictureParameterBufferH264 *pic_param,
> + GenFrameStore frame_store[MAX_GEN_REFERENCE_FRAMES],
> + GenFrameStoreContext *fs_ctx
> +);
>
> void
> gen75_update_avc_frame_store_index(
> diff --git a/src/i965_media_h264.h b/src/i965_media_h264.h
> index 490213c..e507e1d 100644
> --- a/src/i965_media_h264.h
> +++ b/src/i965_media_h264.h
> @@ -61,6 +61,7 @@ struct i965_h264_context
> struct i965_avc_hw_scoreboard_context avc_hw_scoreboard_context;
> struct i965_avc_ildb_context avc_ildb_context;
>
> + GenFrameStoreContext fs_ctx;
> GenFrameStore fsid_list[MAX_GEN_REFERENCE_FRAMES];
>
> struct i965_kernel avc_kernels[NUM_H264_AVC_KERNELS];
> diff --git a/src/intel_media.h b/src/intel_media.h
> index b30740a..55136d6 100644
> --- a/src/intel_media.h
> +++ b/src/intel_media.h
> @@ -39,6 +39,7 @@ struct gen_avc_surface
> dri_bo *dmv_top;
> dri_bo *dmv_bottom;
> int dmv_bottom_flag;
> + int frame_store_id; /* only used for H.264 on earlier generations (<HSW) */
> };
>
> extern void gen_free_avc_surface(void **data);
More information about the Libva
mailing list