Discussion:
[PATCH 2/4] h264: set frame_num in start_frame(), not decode_slice_header()
(too old to reply)
Anton Khirnov
2015-12-05 07:56:54 UTC
Permalink
That is a more appropriate place for it, since it is not allowed to
change between slices.
---
libavcodec/h264_slice.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 4c82588..4f2d6a8 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -553,6 +553,7 @@ static int h264_frame_start(H264Context *h)
pic->reference = h->droppable ? 0 : h->picture_structure;
pic->f->coded_picture_number = h->coded_picture_number++;
pic->field_picture = h->picture_structure != PICT_FRAME;
+ pic->frame_num = h->frame_num;
/*
* Zero key_frame here; IDR markings per slice in frame or fields are ORed
* in later.
@@ -1418,9 +1419,6 @@ int ff_h264_decode_slice_header(H264Context *h, H264SliceContext *sl)
}
}

- if (!h->setup_finished)
- h->cur_pic_ptr->frame_num = h->frame_num; // FIXME frame_num cleanup
-
assert(h->mb_num == h->mb_width * h->mb_height);
if (first_mb_in_slice << FIELD_OR_MBAFF_PICTURE(h) >= h->mb_num ||
first_mb_in_slice >= h->mb_num) {
--
2.0.0
Anton Khirnov
2015-12-05 07:56:55 UTC
Permalink
We do not need to do a full setup like for a real frame, just allocate a
buffer and set cur_pic(_ptr).
---
libavcodec/h264_slice.c | 50 +++++++++++++++++++++++++++++++------------------
1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 4f2d6a8..48d520b 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -290,6 +290,33 @@ static int find_unused_picture(H264Context *h)
return i;
}

+static int initialize_cur_frame(H264Context *h)
+{
+ H264Picture *cur;
+ int ret;
+
+ release_unused_pictures(h, 1);
+ ff_h264_unref_picture(h, &h->cur_pic);
+ h->cur_pic_ptr = NULL;
+
+ ret = find_unused_picture(h);
+ if (ret < 0) {
+ av_log(h->avctx, AV_LOG_ERROR, "no frame buffer available\n");
+ return ret;
+ }
+ cur = &h->DPB[ret];
+
+ ret = alloc_picture(h, cur);
+ if (ret < 0)
+ return ret;
+
+ ret = ff_h264_ref_picture(h, &h->cur_pic, cur);
+ if (ret < 0)
+ return ret;
+ h->cur_pic_ptr = cur;
+
+ return 0;
+}

static void init_dequant8_coeff_table(H264Context *h)
{
@@ -540,16 +567,11 @@ static int h264_frame_start(H264Context *h)
int i, ret;
const int pixel_shift = h->pixel_shift;

- release_unused_pictures(h, 1);
- h->cur_pic_ptr = NULL;
-
- i = find_unused_picture(h);
- if (i < 0) {
- av_log(h->avctx, AV_LOG_ERROR, "no frame buffer available\n");
- return i;
- }
- pic = &h->DPB[i];
+ ret = initialize_cur_frame(h);
+ if (ret < 0)
+ return ret;

+ pic = h->cur_pic_ptr;
pic->reference = h->droppable ? 0 : h->picture_structure;
pic->f->coded_picture_number = h->coded_picture_number++;
pic->field_picture = h->picture_structure != PICT_FRAME;
@@ -563,14 +585,6 @@ static int h264_frame_start(H264Context *h)
pic->mmco_reset = 0;
pic->recovered = 0;

- if ((ret = alloc_picture(h, pic)) < 0)
- return ret;
-
- h->cur_pic_ptr = pic;
- ff_h264_unref_picture(h, &h->cur_pic);
- if ((ret = ff_h264_ref_picture(h, &h->cur_pic, h->cur_pic_ptr)) < 0)
- return ret;
-
if (CONFIG_ERROR_RESILIENCE && h->enable_er)
ff_er_frame_start(&h->slice_ctx[0].er);

@@ -1336,7 +1350,7 @@ int ff_h264_decode_slice_header(H264Context *h, H264SliceContext *sl)
H264Picture *prev = h->short_ref_count ? h->short_ref[0] : NULL;
av_log(h->avctx, AV_LOG_DEBUG, "Frame num gap %d %d\n",
h->frame_num, h->prev_frame_num);
- ret = h264_frame_start(h);
+ ret = initialize_cur_frame(h);
if (ret < 0) {
h->first_field = 0;
return ret;
--
2.0.0
Luca Barbato
2015-12-05 11:25:42 UTC
Permalink
Post by Anton Khirnov
- ff_h264_unref_picture(h, &h->cur_pic);
Seems lost in the refactoring, if it is intentional might be mentioned
in the commit message.

lu
Anton Khirnov
2015-12-06 08:41:52 UTC
Permalink
Quoting Luca Barbato (2015-12-05 12:25:42)
Post by Luca Barbato
Post by Anton Khirnov
- ff_h264_unref_picture(h, &h->cur_pic);
Seems lost in the refactoring, if it is intentional might be mentioned
in the commit message.
It's not lost, just moved higher up.
--
Anton Khirnov
Luca Barbato
2015-12-06 09:21:05 UTC
Permalink
Post by Luca Barbato
Post by Anton Khirnov
- ff_h264_unref_picture(h, &h->cur_pic);
Seems lost in the refactoring, if it is intentional might be mentioned
in the commit message.
lu
_______________________________________________
libav-devel mailing list
https://lists.libav.org/mailman/listinfo/libav-devel
Ah, yes. Looks fine then.

Anton Khirnov
2015-12-05 07:56:56 UTC
Permalink
Fall back to maximum DPB size if the level is unknown.

This should be more spec-compliant and does not depend on the caller
setting has_b_frames before opening the decoder.

The old behaviour, when the delay is supplied by the caller setting
has_b_frames, can still be obtained by setting strict_std_compliance
below normal.
---
libavcodec/h264.c | 15 ++++-----------
libavcodec/h264_ps.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/libavcodec/h264.c b/libavcodec/h264.c
index ec57d6d..6b12b30 100644
--- a/libavcodec/h264.c
+++ b/libavcodec/h264.c
@@ -851,18 +851,11 @@ static void decode_postinit(H264Context *h, int setup_finished)
// FIXME do something with unavailable reference frames

/* Sort B-frames into display order */
-
- if (h->sps.bitstream_restriction_flag &&
- h->avctx->has_b_frames < h->sps.num_reorder_frames) {
- h->avctx->has_b_frames = h->sps.num_reorder_frames;
- h->low_delay = 0;
- }
-
- if (h->avctx->strict_std_compliance >= FF_COMPLIANCE_STRICT &&
- !h->sps.bitstream_restriction_flag) {
- h->avctx->has_b_frames = MAX_DELAYED_PIC_COUNT - 1;
- h->low_delay = 0;
+ if (h->sps.bitstream_restriction_flag ||
+ h->avctx->strict_std_compliance >= FF_COMPLIANCE_NORMAL) {
+ h->avctx->has_b_frames = FFMAX(h->avctx->has_b_frames, h->sps.num_reorder_frames);
}
+ h->low_delay = !h->avctx->has_b_frames;

pics = 0;
while (h->delayed_pic[pics])
diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
index fa9fa7d..6b29966 100644
--- a/libavcodec/h264_ps.c
+++ b/libavcodec/h264_ps.c
@@ -105,6 +105,26 @@ static const uint8_t default_scaling8[2][64] = {
24, 25, 27, 28, 30, 32, 33, 35 }
};

+/* maximum number of MBs in the DPB for a given level */
+static const int level_max_dpb_mbs[][2] = {
+ { 10, 396 },
+ { 11, 900 },
+ { 12, 2376 },
+ { 13, 2376 },
+ { 20, 2376 },
+ { 21, 4752 },
+ { 22, 8100 },
+ { 30, 8100 },
+ { 31, 18000 },
+ { 32, 20480 },
+ { 40, 32768 },
+ { 41, 32768 },
+ { 42, 34816 },
+ { 50, 110400 },
+ { 51, 184320 },
+ { 52, 184320 },
+};
+
static inline int decode_hrd_parameters(H264Context *h, SPS *sps)
{
int cpb_count, i;
@@ -501,6 +521,19 @@ int ff_h264_decode_seq_parameter_set(H264Context *h)
goto fail;
}

+ /* if the maximum delay is not stored in the SPS, derive it based on the
+ * level */
+ if (!sps->bitstream_restriction_flag) {
+ sps->num_reorder_frames = MAX_DELAYED_PIC_COUNT - 1;
+ for (i = 0; i < FF_ARRAY_ELEMS(level_max_dpb_mbs); i++) {
+ if (level_max_dpb_mbs[i][0] == sps->level_idc) {
+ sps->num_reorder_frames = FFMIN(level_max_dpb_mbs[i][1] / (sps->mb_width * sps->mb_height),
+ sps->num_reorder_frames);
+ break;
+ }
+ }
+ }
+
if (!sps->sar.den)
sps->sar.den = 1;
--
2.0.0
Luca Barbato
2015-12-05 11:19:55 UTC
Permalink
Post by Anton Khirnov
Fall back to maximum DPB size if the level is unknown.
This should be more spec-compliant and does not depend on the caller
setting has_b_frames before opening the decoder.
The old behaviour, when the delay is supplied by the caller setting
has_b_frames, can still be obtained by setting strict_std_compliance
below normal.
Sounds good.
Anton Khirnov
2015-12-05 11:12:56 UTC
Permalink
According to the spec, the reference list for a slice should be
constructed by first generating an initial (what we now call "default")
reference list and then optionally applying modifications to it.

Our code has an optimization where the initial reference list is
constructed for the first inter slice and then rebuilt for other slices
if needed. This, however, adds complexity to the code, requires an extra
2.5kB array in the codec context and there is no reason to think that it
has any positive effect on performance. Therefore, simplify the code by
generating the reference list from scratch for each slice.
---
Now actually removing the array
---
libavcodec/h264.h | 2 --
libavcodec/h264_refs.c | 46 +++++++++++++++++++++-------------------------
libavcodec/h264_slice.c | 18 +-----------------
3 files changed, 22 insertions(+), 44 deletions(-)

diff --git a/libavcodec/h264.h b/libavcodec/h264.h
index e9fd9cc..bdb2d24 100644
--- a/libavcodec/h264.h
+++ b/libavcodec/h264.h
@@ -600,7 +600,6 @@ typedef struct H264Context {
*/
int max_pic_num;

- H264Ref default_ref_list[2][32]; ///< base reference list for all slices of a coded picture
H264Picture *short_ref[32];
H264Picture *long_ref[32];
H264Picture *delayed_pic[MAX_DELAYED_PIC_COUNT + 2]; // FIXME size?
@@ -645,7 +644,6 @@ typedef struct H264Context {

enum AVPictureType pict_type;

- int last_slice_type;
/** @} */

/**
diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c
index adc2213..46b51e0 100644
--- a/libavcodec/h264_refs.c
+++ b/libavcodec/h264_refs.c
@@ -119,7 +119,7 @@ static int add_sorted(H264Picture **sorted, H264Picture **src, int len, int limi
return out_i;
}

-int ff_h264_fill_default_ref_list(H264Context *h, H264SliceContext *sl)
+static void h264_initialise_ref_list(H264Context *h, H264SliceContext *sl)
{
int i, len;

@@ -138,52 +138,51 @@ int ff_h264_fill_default_ref_list(H264Context *h, H264SliceContext *sl)
len += add_sorted(sorted + len, h->short_ref, h->short_ref_count, cur_poc, 0 ^ list);
assert(len <= 32);

- len = build_def_list(h->default_ref_list[list], FF_ARRAY_ELEMS(h->default_ref_list[0]),
+ len = build_def_list(sl->ref_list[list], FF_ARRAY_ELEMS(sl->ref_list[0]),
sorted, len, 0, h->picture_structure);
- len += build_def_list(h->default_ref_list[list] + len,
- FF_ARRAY_ELEMS(h->default_ref_list[0]) - len,
+ len += build_def_list(sl->ref_list[list] + len,
+ FF_ARRAY_ELEMS(sl->ref_list[0]) - len,
h->long_ref, 16, 1, h->picture_structure);

if (len < sl->ref_count[list])
- memset(&h->default_ref_list[list][len], 0, sizeof(H264Ref) * (sl->ref_count[list] - len));
+ memset(&sl->ref_list[list][len], 0, sizeof(H264Ref) * (sl->ref_count[list] - len));
lens[list] = len;
}

if (lens[0] == lens[1] && lens[1] > 1) {
for (i = 0; i < lens[0] &&
- h->default_ref_list[0][i].parent->f->buf[0]->buffer ==
- h->default_ref_list[1][i].parent->f->buf[0]->buffer; i++);
+ sl->ref_list[0][i].parent->f->buf[0]->buffer ==
+ sl->ref_list[1][i].parent->f->buf[0]->buffer; i++);
if (i == lens[0]) {
- FFSWAP(H264Ref, h->default_ref_list[1][0], h->default_ref_list[1][1]);
+ FFSWAP(H264Ref, sl->ref_list[1][0], sl->ref_list[1][1]);
}
}
} else {
- len = build_def_list(h->default_ref_list[0], FF_ARRAY_ELEMS(h->default_ref_list[0]),
+ len = build_def_list(sl->ref_list[0], FF_ARRAY_ELEMS(sl->ref_list[0]),
h->short_ref, h->short_ref_count, 0, h->picture_structure);
- len += build_def_list(h->default_ref_list[0] + len,
- FF_ARRAY_ELEMS(h->default_ref_list[0]) - len,
+ len += build_def_list(sl->ref_list[0] + len,
+ FF_ARRAY_ELEMS(sl->ref_list[0]) - len,
h-> long_ref, 16, 1, h->picture_structure);

if (len < sl->ref_count[0])
- memset(&h->default_ref_list[0][len], 0, sizeof(H264Ref) * (sl->ref_count[0] - len));
+ memset(&sl->ref_list[0][len], 0, sizeof(H264Ref) * (sl->ref_count[0] - len));
}
#ifdef TRACE
for (i = 0; i < sl->ref_count[0]; i++) {
ff_tlog(h->avctx, "List0: %s fn:%d 0x%p\n",
- (h->default_ref_list[0][i].long_ref ? "LT" : "ST"),
- h->default_ref_list[0][i].pic_id,
- h->default_ref_list[0][i].f->data[0]);
+ (sl->ref_list[0][i].long_ref ? "LT" : "ST"),
+ sl->ref_list[0][i].pic_id,
+ sl->ref_list[0][i].f->data[0]);
}
if (sl->slice_type_nos == AV_PICTURE_TYPE_B) {
for (i = 0; i < sl->ref_count[1]; i++) {
ff_tlog(h->avctx, "List1: %s fn:%d 0x%p\n",
- (h->default_ref_list[1][i].long_ref ? "LT" : "ST"),
- h->default_ref_list[1][i].pic_id,
- h->default_ref_list[1][i].f->data[0]);
+ (sl->ref_list[1][i].long_ref ? "LT" : "ST"),
+ sl->ref_list[1][i].pic_id,
+ sl->ref_list[1][i].f->data[0]);
}
}
#endif
- return 0;
}

static void print_short_term(H264Context *h);
@@ -219,9 +218,9 @@ int ff_h264_decode_ref_pic_list_reordering(H264Context *h, H264SliceContext *sl)
print_short_term(h);
print_long_term(h);

- for (list = 0; list < sl->list_count; list++) {
- memcpy(sl->ref_list[list], h->default_ref_list[list], sl->ref_count[list] * sizeof(sl->ref_list[0][0]));
+ h264_initialise_ref_list(h, sl);

+ for (list = 0; list < sl->list_count; list++) {
if (get_bits1(&sl->gb)) { // ref_pic_list_modification_flag_l[01]
int pred = h->curr_pic_num;

@@ -326,10 +325,7 @@ int ff_h264_decode_ref_pic_list_reordering(H264Context *h, H264SliceContext *sl)
for (index = 0; index < sl->ref_count[list]; index++) {
if (!sl->ref_list[list][index].parent) {
av_log(h->avctx, AV_LOG_ERROR, "Missing reference picture\n");
- if (h->default_ref_list[list][0].parent)
- sl->ref_list[list][index] = h->default_ref_list[list][0];
- else
- return -1;
+ return -1;
}
}
}
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 49fd891..4c82588 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -510,18 +510,13 @@ int ff_h264_update_thread_context(AVCodecContext *dst,
h->dequant_coeff_pps = h1->dequant_coeff_pps;

// POC timing
- copy_fields(h, h1, poc_lsb, default_ref_list);
-
- // reference lists
- copy_fields(h, h1, short_ref, current_slice);
+ copy_fields(h, h1, poc_lsb, current_slice);

copy_picture_range(h->short_ref, h1->short_ref, 32, h, h1);
copy_picture_range(h->long_ref, h1->long_ref, 32, h, h1);
copy_picture_range(h->delayed_pic, h1->delayed_pic,
MAX_DELAYED_PIC_COUNT + 2, h, h1);

- h->last_slice_type = h1->last_slice_type;
-
if (!h->cur_pic_ptr)
return 0;

@@ -1032,7 +1027,6 @@ int ff_h264_decode_slice_header(H264Context *h, H264SliceContext *sl)
unsigned int pps_id;
int ret;
unsigned int slice_type, tmp, i, j;
- int default_ref_list_done = 0;
int last_pic_structure, last_pic_droppable;
int needs_reinit = 0;
int field_pic_flag, bottom_field_flag;
@@ -1073,10 +1067,6 @@ int ff_h264_decode_slice_header(H264Context *h, H264SliceContext *sl)
sl->slice_type_fixed = 0;

slice_type = golomb_to_pict_type[slice_type];
- if (slice_type == AV_PICTURE_TYPE_I ||
- (h->current_slice != 0 && slice_type == h->last_slice_type)) {
- default_ref_list_done = 1;
- }
sl->slice_type = slice_type;
sl->slice_type_nos = slice_type & 3;

@@ -1491,11 +1481,6 @@ int ff_h264_decode_slice_header(H264Context *h, H264SliceContext *sl)
ret = ff_set_ref_count(h, sl);
if (ret < 0)
return ret;
- else if (ret == 1)
- default_ref_list_done = 0;
-
- if (!default_ref_list_done)
- ff_h264_fill_default_ref_list(h, sl);

if (sl->slice_type_nos != AV_PICTURE_TYPE_I) {
ret = ff_h264_decode_ref_pic_list_reordering(h, sl);
@@ -1635,7 +1620,6 @@ int ff_h264_decode_slice_header(H264Context *h, H264SliceContext *sl)
h->pps.chroma_qp_index_offset[1]) +
6 * (h->sps.bit_depth_luma - 8);

- h->last_slice_type = slice_type;
sl->slice_num = ++h->current_slice;
if (sl->slice_num >= MAX_SLICES) {
av_log(h->avctx, AV_LOG_ERROR,
--
2.0.0
Luca Barbato
2015-12-05 11:17:28 UTC
Permalink
Post by Anton Khirnov
According to the spec, the reference list for a slice should be
constructed by first generating an initial (what we now call "default")
reference list and then optionally applying modifications to it.
Our code has an optimization where the initial reference list is
constructed for the first inter slice and then rebuilt for other slices
if needed. This, however, adds complexity to the code, requires an extra
2.5kB array in the codec context and there is no reason to think that it
has any positive effect on performance. Therefore, simplify the code by
generating the reference list from scratch for each slice.
---
Now actually removing the array
---
libavcodec/h264.h | 2 --
libavcodec/h264_refs.c | 46 +++++++++++++++++++++-------------------------
libavcodec/h264_slice.c | 18 +-----------------
3 files changed, 22 insertions(+), 44 deletions(-)
Fine by me.
Luca Barbato
2015-12-05 11:18:04 UTC
Permalink
Post by Anton Khirnov
That is a more appropriate place for it, since it is not allowed to
change between slices.
---
libavcodec/h264_slice.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
Sounds fine.
Continue reading on narkive:
Loading...