[FFmpeg-devel] [PATCH] h264: don't clobber mmco opcode tables for non-first slice headers.
Michael Niedermayer
michaelni at gmx.at
Mon Jan 14 04:36:05 CET 2013
On Sun, Jan 13, 2013 at 04:40:24PM -0800, Ronald S. Bultje wrote:
> From: "Ronald S. Bultje" <rsbultje at gmail.com>
>
> Clobbering these tables will temporarily clobber the template used
> as a basis for other threads to start decoding from. If the other
> decoding thread updates from the template right at that moment,
> subsequent threads will get invalid (or, usually, none at all) mmco
> tables. This leads to invalid reference lists and subsequent decode
> failures.
>
> Therefore, instead, decode the mmco tables only for the first slice in
> a field or frame. For other slices, decode the bits and ensure they
> are identical to the mmco tables in the first slice, but don't ever
> clobber the context state. This prevents other threads from using a
> clobbered/invalid template as starting point for decoding, and thus
> fixes decoding in these cases.
>
> This fixes occasional (~1%) failures of h264-conformance-mr1_bt_a with
> frame-multithreading enabled.
> diff --git a/libavcodec/h264.c b/libavcodec/h264.c
> index d8d438e..eb28fe9 100644
> --- a/libavcodec/h264.c
> +++ b/libavcodec/h264.c
> @@ -3159,7 +3159,9 @@ static int decode_slice_header(H264Context *h, H264Context *h0)
> }
> }
>
> - if (h->nal_ref_idc && ff_h264_decode_ref_pic_marking(h0, &s->gb) < 0 &&
> + if (h->nal_ref_idc &&
> + ff_h264_decode_ref_pic_marking(h0, &s->gb,
> + first_mb_in_slice == 0) < 0 &&
This depends on the first slice not being lost, if it is lost this
will fail. It also will fail with ASO
I suggest you check for the first slice differently, that is the
first slice that we actually do have instead of the one which
contains MB #1
a check like current_slice == 0 might work but it still has another
problem, that is if a slice is there but damaged.
I think most robust would be to set a flag once the decoding of the
lists has succeeded and then only check subsequent slices once a
correctly decoded list is available.
(this would complicate threads a bit though as setup of the next
would need to wait)
but at least in the absence of threads, a missing or damaged first
slice should not cause a failure.
> (s->avctx->err_recognition & AV_EF_EXPLODE))
> return AVERROR_INVALIDDATA;
>
> diff --git a/libavcodec/h264.h b/libavcodec/h264.h
> index 3355d05..df462f1 100644
> --- a/libavcodec/h264.h
> +++ b/libavcodec/h264.h
> @@ -669,7 +669,8 @@ void ff_h264_remove_all_refs(H264Context *h);
> */
> int ff_h264_execute_ref_pic_marking(H264Context *h, MMCO *mmco, int mmco_count);
>
> -int ff_h264_decode_ref_pic_marking(H264Context *h, GetBitContext *gb);
> +int ff_h264_decode_ref_pic_marking(H264Context *h, GetBitContext *gb,
> + int first_slice);
>
> void ff_generate_sliding_window_mmcos(H264Context *h);
>
>
> diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c
> index 812913a..e00a5e7 100644
> --- a/libavcodec/h264_refs.c
> +++ b/libavcodec/h264_refs.c
> @@ -665,51 +665,94 @@ int ff_h264_execute_ref_pic_marking(H264Context *h, MMCO *mmco, int mmco_count){
> return (h->s.avctx->err_recognition & AV_EF_EXPLODE) ? err : 0;
> }
>
> +static int check_opcodes(MMCO *mmco1, MMCO *mmco2, int n_mmcos)
> +{
> + int i;
> +
> + for (i = 0; i < n_mmcos; i++) {
> + if (mmco1[i].opcode != mmco2[i].opcode)
> + return -1;
> + }
> +
> + return 0;
> +}
this only checks the opcodes, the other fields should be checked
too, also i wonder why you dont use a memcmp ?
> +
> -int ff_h264_decode_ref_pic_marking(H264Context *h, GetBitContext *gb){
> +int ff_h264_decode_ref_pic_marking(H264Context *h, GetBitContext *gb,
> + int first_slice)
> +{
> MpegEncContext * const s = &h->s;
> int i;
> + MMCO mmco_temp[MAX_MMCO_COUNT], *mmco = first_slice ? h->mmco : mmco_temp;
> + int mmco_index = 0;
>
> - h->mmco_index= 0;
> if(h->nal_unit_type == NAL_IDR_SLICE){ //FIXME fields
> s->broken_link= get_bits1(gb) -1;
> if(get_bits1(gb)){
> - h->mmco[0].opcode= MMCO_LONG;
> - h->mmco[0].long_arg= 0;
> - h->mmco_index= 1;
> + mmco[0].opcode = MMCO_LONG;
> + mmco[0].long_arg = 0;
> + mmco_index = 1;
> }
> }else{
> if(get_bits1(gb)){ // adaptive_ref_pic_marking_mode_flag
> for(i= 0; i<MAX_MMCO_COUNT; i++) {
> MMCOOpcode opcode= get_ue_golomb_31(gb);
>
> - h->mmco[i].opcode= opcode;
> + mmco[i].opcode = opcode;
> if(opcode==MMCO_SHORT2UNUSED || opcode==MMCO_SHORT2LONG){
> - h->mmco[i].short_pic_num= (h->curr_pic_num - get_ue_golomb(gb) - 1) & (h->max_pic_num - 1);
> + mmco[i].short_pic_num =
> + (h->curr_pic_num - get_ue_golomb(gb) - 1) &
> + (h->max_pic_num - 1);
[... disabled code sniped ...]
> - }
> - if(opcode==MMCO_SHORT2LONG || opcode==MMCO_LONG2UNUSED || opcode==MMCO_LONG || opcode==MMCO_SET_MAX_LONG){
> + }
> + if (opcode == MMCO_SHORT2LONG || opcode == MMCO_LONG2UNUSED ||
> + opcode == MMCO_LONG || opcode == MMCO_SET_MAX_LONG) {
unrelated reformating that makes the patch harder to review
> unsigned int long_arg= get_ue_golomb_31(gb);
> - if(long_arg >= 32 || (long_arg >= 16 && !(opcode == MMCO_SET_MAX_LONG && long_arg == 16) && !(opcode == MMCO_LONG2UNUSED && FIELD_PICTURE))){
> - av_log(h->s.avctx, AV_LOG_ERROR, "illegal long ref in memory management control operation %d\n", opcode);
> + if (long_arg >= 32 ||
> + (long_arg >= 16 && !(opcode == MMCO_SET_MAX_LONG &&
> + long_arg == 16) &&
> + !(opcode == MMCO_LONG2UNUSED && FIELD_PICTURE))){
> + av_log(h->s.avctx, AV_LOG_ERROR,
> + "illegal long ref in memory management control "
> + "operation %d\n", opcode);
> return -1;
> }
> - h->mmco[i].long_arg= long_arg;
> + mmco[i].long_arg = long_arg;
> }
>
> if(opcode > (unsigned)MMCO_LONG){
> - av_log(h->s.avctx, AV_LOG_ERROR, "illegal memory management control operation %d\n", opcode);
> + av_log(h->s.avctx, AV_LOG_ERROR,
> + "illegal memory management control operation %d\n",
> + opcode);
> return -1;
> }
> if(opcode == MMCO_END)
> break;
> }
> - h->mmco_index= i;
> + mmco_index = i;
> }else{
> + if (first_slice)
> ff_generate_sliding_window_mmcos(h);
> + mmco_index = -1;
you skip checking the sliding window mmcos, they can mismatch too
but overall nice work and thanks alot for debuging this !
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130114/272bb4ec/attachment.asc>
More information about the ffmpeg-devel
mailing list