2014-06-09 15:57 GMT+02:00 Xiang, Haihao <haihao.xi...@intel.com>:
>
> I am OK with the patch if you can remove the redundant line (see below) when
> You push the code.
>
> Thanks
> Haihao
>
>> -----Original Message-----
>> From: Libva [mailto:libva-boun...@lists.freedesktop.org] On Behalf Of
>> Gwenole Beauchesne
>> Sent: Friday, June 06, 2014 5:16 PM
>> To: libva@lists.freedesktop.org
>> Subject: [Libva] [PATCH v5 intel-driver 11/11] decoder: h264: fix frame store
>> logic for MVC.
>>
>> 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.
>>
>> Signed-off-by: Gwenole Beauchesne <gwenole.beauche...@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 is always updated after else if (){}, so we can remove the
> above redundant line in else if () {}.
OK, we could even reduce this to:
if (fs_ctx->ref_age == 0 || fs_ctx->prev_poc != poc)
fs_ctx->ref_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);
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Libva mailing list
>> Libva@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/libva
_______________________________________________
Libva mailing list
Libva@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libva
