On Sun, 8 Aug 2021, Michael Niedermayer wrote:

On Sat, Aug 07, 2021 at 08:32:35PM +0200, Marton Balint wrote:


On Sat, 7 Aug 2021, Michael Niedermayer wrote:

On Sat, Aug 07, 2021 at 09:21:36AM +0200, Marton Balint wrote:


On Fri, 6 Aug 2021, Michael Niedermayer wrote:

On Mon, Aug 02, 2021 at 08:50:05PM +0200, Marton Balint wrote:
Also split error message to error and warning.

Signed-off-by: Marton Balint <c...@passwd.hu>
---
 libavcodec/mpeg12dec.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 858dca660c..49d865853b 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -1529,7 +1529,7 @@ static void 
mpeg_decode_quant_matrix_extension(MpegEncContext *s)
         load_matrix(s, s->chroma_inter_matrix, NULL, 0);
 }

-static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1)
+static int mpeg_decode_picture_coding_extension(Mpeg1Context *s1)
 {
     MpegEncContext *s = &s1->mpeg_enc_ctx;

@@ -1539,8 +1539,10 @@ static void 
mpeg_decode_picture_coding_extension(Mpeg1Context *s1)
     s->mpeg_f_code[1][0] = get_bits(&s->gb, 4);
     s->mpeg_f_code[1][1] = get_bits(&s->gb, 4);
     if (!s->pict_type && s1->mpeg_enc_ctx_allocated) {
-        av_log(s->avctx, AV_LOG_ERROR,
-               "Missing picture start code, guessing missing values\n");
+        av_log(s->avctx, AV_LOG_ERROR, "Missing picture start code\n");
+        if (s->avctx->err_recognition & AV_EF_EXPLODE)
+            return AVERROR_INVALIDDATA;
+        av_log(s->avctx, AV_LOG_WARNING, "Guessing pict_type from 
mpeg_f_code\n");

If we are nitpicking then this is not ideal
"Missing picture start code" is only an error when AV_EF_EXPLODE is set
because it only in that case results in wrong output, when AV_EF_EXPLODE
is not set the output from the decoder might be correct si it too should
be a warning in that case

I understand your logic, but there are arguments for keeping the error
message as well. I think in practice, most times, this error is caused by
bitstream errors and it _will_ affect decoding.

Also there are tons of cases
in the code which log an error, then return INVALIDDATA if EXPLODE is used,
so why handle it here differently...

do these cases really represent situations where the output might be correct ?
for examlple any out of range coefficient cant be correct and a mismatching CRC
well it "could" be.
I think a big part would be, "do we actually have cases where it is not an
error ?" if so i would argue that it should be a warning

Obviously it is hard to be sure. Among the samples we have, I found a few
files affected:

ffmpeg-bugs/trac/ticket2705/81391.mpg
ffmpeg-bugs/trac/ticket2950/mpeg2_fuzz.mpg
ffmpeg-bugs/roundup/issue1872/lol-mpeg2.mp4
ffmpeg-bugs/roundup/issue1218/dvbt-ffmpeg-seg-fault.ts

All are heavily corrupted files.

I also had a private sample file having this issue, and no other error but
the "ignoring extra picture following a frame-picture" warning, and it is
also a corrupted bitstream, and causes missed frames when decoding.


So a patologcal bitstream can probably be created when the decoding is not
affected, but my guess is that for real-world files decoding is always
affected, therefore I think an error is more appropriate.

i thought i saw one where it was just missing and no corruption but maybe
not. Its certainly not worth the time spent on this to search further



Let me know what you prefer.

please use what you prefer

Ok, applied my version then.

Thanks,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Reply via email to