[FFmpeg-devel] [PATCH v2] avcodec/mpeg12dec: report error when picture type is unknown and err_detect is EXPLODE

[Michael Niedermayer]

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 <cus at 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

> 
> But let me know if you insisit and I will change it. And yes, it is indeed
> nitpicking :)

i dont insist but thats how the levels are defined

/**
 * Something went wrong and cannot losslessly be recovered.
 * However, not all future data is affected.
 */
#define AV_LOG_ERROR    16

/**
 * Something somehow does not look correct. This may or may not
 * lead to problems. An example would be the use of '-vstrict -2'.
 */
#define AV_LOG_WARNING  24


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"I am not trying to be anyone's saviour, I'm trying to think about the
 future and not be sad" - Elon Musk

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210807/db97888d/attachment.sig>