[FFmpeg-devel] set AVFrame decode_error_flags in case of decoding error by h264dec

Submitted by Amir Pauker on June 10, 2019, 5:45 a.m.

Details

Message ID 1560145513-2130-1-git-send-email-amir@livelyvideo.tv
State New
Headers show

Commit Message

Amir Pauker June 10, 2019, 5:45 a.m.
set AVFrame decode_error_flags in case h->slice_ctx->er.error_occurred is set
after the call to ff_h264_execute_decode_slices. This allows the user to detect
concealed decoding errors in the call to avcodec_receive_frame

Signed-off-by: Amir Pauker <amir@livelyvideo.tv>
---
 libavcodec/h264dec.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Michael Niedermayer June 11, 2019, 4:39 p.m.
On Sun, Jun 09, 2019 at 10:45:13PM -0700, Amir Pauker wrote:
> set AVFrame decode_error_flags in case h->slice_ctx->er.error_occurred is set
> after the call to ff_h264_execute_decode_slices. This allows the user to detect
> concealed decoding errors in the call to avcodec_receive_frame
> 
> Signed-off-by: Amir Pauker <amir@livelyvideo.tv>
> ---
>  libavcodec/h264dec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> index 00d922f..67dee11 100644
> --- a/libavcodec/h264dec.c
> +++ b/libavcodec/h264dec.c
> @@ -758,6 +758,12 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size)
>      }
>  
>      ret = ff_h264_execute_decode_slices(h);
> +
> +    // set decode_error_flags to allow users to detect concealed decoding errors
> +    if( (ret < 0 || h->slice_ctx->er.error_occurred) && h->cur_pic_ptr){
> +        h->cur_pic_ptr->f->decode_error_flags |= FF_DECODE_ERROR_INVALID_BITSTREAM|FF_DECODE_ERROR_MISSING_REFERENCE;
> +    }

This is not correct.
error_occurred does not imply the 2 specific errors

[...]
Amir Pauker June 11, 2019, 8:21 p.m.
Thanks Michael Niedermayer for looking into this

What I am trying to solve is having a way to detect concealed decoding
errors by the caller to avcodec_receive_frame.

Should I add a general value e.g. #define
FF_DECODE_ERROR_DECODE_ERROR_OCCURRED 4 ?

Thanks
Amir

On Tue, Jun 11, 2019 at 11:39 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Sun, Jun 09, 2019 at 10:45:13PM -0700, Amir Pauker wrote:
> > set AVFrame decode_error_flags in case h->slice_ctx->er.error_occurred
> is set
> > after the call to ff_h264_execute_decode_slices. This allows the user to
> detect
> > concealed decoding errors in the call to avcodec_receive_frame
> >
> > Signed-off-by: Amir Pauker <amir@livelyvideo.tv>
> > ---
> >  libavcodec/h264dec.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> > index 00d922f..67dee11 100644
> > --- a/libavcodec/h264dec.c
> > +++ b/libavcodec/h264dec.c
> > @@ -758,6 +758,12 @@ static int decode_nal_units(H264Context *h, const
> uint8_t *buf, int buf_size)
> >      }
> >
> >      ret = ff_h264_execute_decode_slices(h);
> > +
> > +    // set decode_error_flags to allow users to detect concealed
> decoding errors
> > +    if( (ret < 0 || h->slice_ctx->er.error_occurred) && h->cur_pic_ptr){
> > +        h->cur_pic_ptr->f->decode_error_flags |=
> FF_DECODE_ERROR_INVALID_BITSTREAM|FF_DECODE_ERROR_MISSING_REFERENCE;
> > +    }
>
> This is not correct.
> error_occurred does not imply the 2 specific errors
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> What does censorship reveal? It reveals fear. -- Julian Assange
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer June 12, 2019, 7:48 a.m.
On Tue, Jun 11, 2019 at 03:21:41PM -0500, Amir Z wrote:
> Thanks Michael Niedermayer for looking into this
> 
> What I am trying to solve is having a way to detect concealed decoding
> errors by the caller to avcodec_receive_frame.
> 
> Should I add a general value e.g. #define
> FF_DECODE_ERROR_DECODE_ERROR_OCCURRED 4 ?

I suggest 
FF_DECODE_ERROR_CONCEALMENT_ACTIVE or something similar and then always
set this for all cases of error concealment
Its more informative than just knowing there was an error

also dont forget to increase minor version and add a entry to APICHanges

thx

[...]
Marton Balint June 12, 2019, 8:09 a.m.
On Wed, 12 Jun 2019, Michael Niedermayer wrote:

> On Tue, Jun 11, 2019 at 03:21:41PM -0500, Amir Z wrote:
>> Thanks Michael Niedermayer for looking into this
>>
>> What I am trying to solve is having a way to detect concealed decoding
>> errors by the caller to avcodec_receive_frame.
>>
>> Should I add a general value e.g. #define
>> FF_DECODE_ERROR_DECODE_ERROR_OCCURRED 4 ?
>
> I suggest
> FF_DECODE_ERROR_CONCEALMENT_ACTIVE or something similar and then always
> set this for all cases of error concealment
> Its more informative than just knowing there was an error

Concealment is a consequence. Error_flags should refer to the cause. A 
generic UNKNOWN error seems much better to me if it is not feasible to 
determine the cause.

Regards,
Marton
Michael Niedermayer June 12, 2019, 9:34 a.m.
On Wed, Jun 12, 2019 at 10:09:08AM +0200, Marton Balint wrote:
> 
> 
> On Wed, 12 Jun 2019, Michael Niedermayer wrote:
> 
> >On Tue, Jun 11, 2019 at 03:21:41PM -0500, Amir Z wrote:
> >>Thanks Michael Niedermayer for looking into this
> >>
> >>What I am trying to solve is having a way to detect concealed decoding
> >>errors by the caller to avcodec_receive_frame.
> >>
> >>Should I add a general value e.g. #define
> >>FF_DECODE_ERROR_DECODE_ERROR_OCCURRED 4 ?
> >
> >I suggest
> >FF_DECODE_ERROR_CONCEALMENT_ACTIVE or something similar and then always
> >set this for all cases of error concealment
> >Its more informative than just knowing there was an error
> 
> Concealment is a consequence. Error_flags should refer to the cause. A
> generic UNKNOWN error seems much better to me if it is not feasible to
> determine the cause.

Concealment is the consequence generally, still the error in the frames
differs between concealment or no concealment. A user application may
want to treat these differently.
concealemnt is not supported by all codecs currently and also not by
all variants, for example interlaced material tends to be less supported
in concealment. A user app might choose to discard a frame that
contains errors but no concealemnt if the following frame is fine.

Also in a very pedantic view, concealemnt itself is an error too.
Its rarly known exactly where the damage starts so concealment often needs
to cover more and by doing so adds errors in a minority of locations
that is if you just want a formal argument why this would fit in here.
Not an argument against the principle that concealemnt differs here in what
it is, you are certainly correct about that.

[...]
Amir Pauker June 12, 2019, 1:58 p.m.
FF_DECODE_ERROR_CONCEALMENT_ACTIVE sounds right for the case that the
ret variable is set to zero (i.e. indicate the fact that there was an
error and it is concealed)

    ret = ff_h264_execute_decode_slices(h);
    if (ret < 0 && (h->avctx->err_recognition & AV_EF_EXPLODE))
        goto end;

    // set h->cur_pic_ptr->f->decode_error_flags here

    ret = 0;
end:

Thank


On Wed, Jun 12, 2019 at 4:35 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Wed, Jun 12, 2019 at 10:09:08AM +0200, Marton Balint wrote:
> >
> >
> > On Wed, 12 Jun 2019, Michael Niedermayer wrote:
> >
> > >On Tue, Jun 11, 2019 at 03:21:41PM -0500, Amir Z wrote:
> > >>Thanks Michael Niedermayer for looking into this
> > >>
> > >>What I am trying to solve is having a way to detect concealed decoding
> > >>errors by the caller to avcodec_receive_frame.
> > >>
> > >>Should I add a general value e.g. #define
> > >>FF_DECODE_ERROR_DECODE_ERROR_OCCURRED 4 ?
> > >
> > >I suggest
> > >FF_DECODE_ERROR_CONCEALMENT_ACTIVE or something similar and then always
> > >set this for all cases of error concealment
> > >Its more informative than just knowing there was an error
> >
> > Concealment is a consequence. Error_flags should refer to the cause. A
> > generic UNKNOWN error seems much better to me if it is not feasible to
> > determine the cause.
>
> Concealment is the consequence generally, still the error in the frames
> differs between concealment or no concealment. A user application may
> want to treat these differently.
> concealemnt is not supported by all codecs currently and also not by
> all variants, for example interlaced material tends to be less supported
> in concealment. A user app might choose to discard a frame that
> contains errors but no concealemnt if the following frame is fine.
>
> Also in a very pedantic view, concealemnt itself is an error too.
> Its rarly known exactly where the damage starts so concealment often needs
> to cover more and by doing so adds errors in a minority of locations
> that is if you just want a formal argument why this would fit in here.
> Not an argument against the principle that concealemnt differs here in what
> it is, you are certainly correct about that.
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Modern terrorism, a quick summary: Need oil, start war with country that
> has oil, kill hundread thousand in war. Let country fall into chaos,
> be surprised about raise of fundamantalists. Drop more bombs, kill more
> people, be surprised about them taking revenge and drop even more bombs
> and strip your own citizens of their rights and freedoms. to be continued
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint June 12, 2019, 7:59 p.m.
On Wed, 12 Jun 2019, Michael Niedermayer wrote:

> On Wed, Jun 12, 2019 at 10:09:08AM +0200, Marton Balint wrote:
>>
>>
>> On Wed, 12 Jun 2019, Michael Niedermayer wrote:
>>
>>> On Tue, Jun 11, 2019 at 03:21:41PM -0500, Amir Z wrote:
>>>> Thanks Michael Niedermayer for looking into this
>>>>
>>>> What I am trying to solve is having a way to detect concealed decoding
>>>> errors by the caller to avcodec_receive_frame.
>>>>
>>>> Should I add a general value e.g. #define
>>>> FF_DECODE_ERROR_DECODE_ERROR_OCCURRED 4 ?
>>>
>>> I suggest
>>> FF_DECODE_ERROR_CONCEALMENT_ACTIVE or something similar and then always
>>> set this for all cases of error concealment
>>> Its more informative than just knowing there was an error
>>
>> Concealment is a consequence. Error_flags should refer to the cause. A
>> generic UNKNOWN error seems much better to me if it is not feasible to
>> determine the cause.
>
> Concealment is the consequence generally, still the error in the frames
> differs between concealment or no concealment. A user application may
> want to treat these differently.
> concealemnt is not supported by all codecs currently and also not by
> all variants, for example interlaced material tends to be less supported
> in concealment. A user app might choose to discard a frame that
> contains errors but no concealemnt if the following frame is fine.
>
> Also in a very pedantic view, concealemnt itself is an error too.
> Its rarly known exactly where the damage starts so concealment often needs
> to cover more and by doing so adds errors in a minority of locations
> that is if you just want a formal argument why this would fit in here.
> Not an argument against the principle that concealemnt differs here in what
> it is, you are certainly correct about that.

OK, FF_DECODE_ERROR_CONCEALMENT_ACTIVE is fine then.

Thanks,
Marton
Amir Pauker June 12, 2019, 10:24 p.m.
thanks, I will submit a new patch with the requested changes

On Wed, Jun 12, 2019 at 3:00 PM Marton Balint <cus@passwd.hu> wrote:

>
>
> On Wed, 12 Jun 2019, Michael Niedermayer wrote:
>
> > On Wed, Jun 12, 2019 at 10:09:08AM +0200, Marton Balint wrote:
> >>
> >>
> >> On Wed, 12 Jun 2019, Michael Niedermayer wrote:
> >>
> >>> On Tue, Jun 11, 2019 at 03:21:41PM -0500, Amir Z wrote:
> >>>> Thanks Michael Niedermayer for looking into this
> >>>>
> >>>> What I am trying to solve is having a way to detect concealed decoding
> >>>> errors by the caller to avcodec_receive_frame.
> >>>>
> >>>> Should I add a general value e.g. #define
> >>>> FF_DECODE_ERROR_DECODE_ERROR_OCCURRED 4 ?
> >>>
> >>> I suggest
> >>> FF_DECODE_ERROR_CONCEALMENT_ACTIVE or something similar and then always
> >>> set this for all cases of error concealment
> >>> Its more informative than just knowing there was an error
> >>
> >> Concealment is a consequence. Error_flags should refer to the cause. A
> >> generic UNKNOWN error seems much better to me if it is not feasible to
> >> determine the cause.
> >
> > Concealment is the consequence generally, still the error in the frames
> > differs between concealment or no concealment. A user application may
> > want to treat these differently.
> > concealemnt is not supported by all codecs currently and also not by
> > all variants, for example interlaced material tends to be less supported
> > in concealment. A user app might choose to discard a frame that
> > contains errors but no concealemnt if the following frame is fine.
> >
> > Also in a very pedantic view, concealemnt itself is an error too.
> > Its rarly known exactly where the damage starts so concealment often
> needs
> > to cover more and by doing so adds errors in a minority of locations
> > that is if you just want a formal argument why this would fit in here.
> > Not an argument against the principle that concealemnt differs here in
> what
> > it is, you are certainly correct about that.
>
> OK, FF_DECODE_ERROR_CONCEALMENT_ACTIVE is fine 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-request@ffmpeg.org with subject "unsubscribe".

Patch hide | download patch | download mbox

diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
index 00d922f..67dee11 100644
--- a/libavcodec/h264dec.c
+++ b/libavcodec/h264dec.c
@@ -758,6 +758,12 @@  static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size)
     }
 
     ret = ff_h264_execute_decode_slices(h);
+
+    // set decode_error_flags to allow users to detect concealed decoding errors
+    if( (ret < 0 || h->slice_ctx->er.error_occurred) && h->cur_pic_ptr){
+        h->cur_pic_ptr->f->decode_error_flags |= FF_DECODE_ERROR_INVALID_BITSTREAM|FF_DECODE_ERROR_MISSING_REFERENCE;
+    }
+
     if (ret < 0 && (h->avctx->err_recognition & AV_EF_EXPLODE))
         goto end;