Post by Mark ThompsonPost by Mark ThompsonPost by Paul B MaholPost by Michael NiedermayerPost by Paul B MaholPost by Michael NiedermayerPost by Paul B MaholThis recovers state with #7374 linked sample.
Work funded by Open Broadcast Systems.
---
libavcodec/h264_refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c
index eaf965e43d..5645a203a7 100644
--- a/libavcodec/h264_refs.c
+++ b/libavcodec/h264_refs.c
@@ -718,6 +718,7 @@ int
ff_h264_execute_ref_pic_marking(H264Context
*h)
}
break;
while (h->short_ref_count) {
remove_short(h, h->short_ref[0]->frame_num, 0);
}
@@ -730,7 +731,6 @@ int
ff_h264_execute_ref_pic_marking(H264Context
*h)
for (j = 0; j < MAX_DELAYED_PIC_COUNT; j++)
h->last_pocs[j] = INT_MIN;
break;
- default: assert(0);
}
}
mmco[i].opcode should not be invalid, its checked around the point
where
this
array is filled.
unless there is something iam missing
Yes, you are missing big time.
If you think by "checked" about those nice asserts they are not
enabled
at
all.
There is check for invalid opcode, but stored invalid opcode is not changed.
Theres no question that we end with a invalid value in the struct,
but
that
is not intended to be in there. You can see that this is not intended by
simply looking at the variable that holds the number of entries, it is
not written at all in this case.
So for example if this code stores 5 correct looking mmcos and the
6th
is
invalid, 6 are in the array but the number of entries is just left
where
it
was, that could be maybe 3 or 8 or 1. Just "defaulting out" the
invalid
value
later doesnt feel ideal.
Nope, mmco state is left in inconsistent state, and my patch fix it.
As
you
provided nothing valuable to consider as alternative I will apply it.
What about converting any invalid mmco opcode to mmco reset and
updating mmco size too?
Currently mmco size is left in previous state.
Don't invent a new RESET (= 5) operation - that type is special and its
presence implies other constraints which we probably don't want to think
about here (around frame_num in particular).
I think END / truncating the list would be best option?
Nope, that would still put it into bad state. With your approach decoder does
not recover from artifacts. Try sample from bug report #7374.
Adding a spurious reset here throws away all previous reference frames,
which will break the stream where it wouldn't otherwise be if the
corrupted frame would have been bypassed for referencing. For example, in
real-time cases with feedback a stream which has encountered errors can be
recovered without an intra frame by referring to an older frame which
still exists in the DPB.
Sample: <http://ixia.jkqxz.net/~mrt/ffmpeg/no-intra.264>. The bad frame
here has frame_num 24, but we already hit an error before that point and the
feedback about that makes frame_num 25 refer to LTRF 1 such that 24 is never
used. (From a simulator rather than a real capture, because random bit
errors are never where you want them.)
It doesn't exactly hit the original issue because the leftover MMCO count
diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c
index 5645a203a7..977b4ed12f 100644
--- a/libavcodec/h264_refs.c
+++ b/libavcodec/h264_refs.c
@@ -875,6 +875,7 @@ int ff_h264_decode_ref_pic_marking(H264SliceContext *sl,
GetBitContext *gb,
av_log(logctx, AV_LOG_ERROR,
"illegal memory management control operation %d\n",
opcode);
+ sl->nb_mmco = i + 1;
return -1;
}
if (opcode == MMCO_END)
to make sure the MMCO count is written with a high enough value it does.
The decoder then loses sync after the inserted reset when that is present
and so all frames are wrong, while without the reset sync is maintained and
all errors are fixed within a few round trips.
Your change doesn't fix the issue in question...