[MPlayer-dev-eng] [PATCH v3] loosen requirements on error resilence flags

[Alexander Strasser]

Hi Reinhard,
Hi Reimar,

Reimar Döffinger wrote:
> On Fri, Feb 10, 2012 at 09:48:36PM +0100, Reimar Döffinger wrote:
> > On Fri, Feb 10, 2012 at 09:23:44PM +0100, Reinhard Tartler wrote:
> > > On Fri, Feb 10, 2012 at 7:41 PM, Reimar Döffinger
> > > <Reimar.Doeffinger at gmx.de> wrote:
> > > >
> > > >
> > > > On 10 Feb 2012, at 19:19, Reinhard Tartler <siretart at gmail.com> wrote:
> > > >
> > > >> On Mon, Feb 6, 2012 at 7:32 AM, Reinhard Tartler <siretart at gmail.com> wrote:
> > > >>> $subj
> > > >>
> > > >> Variant two.
> > > >>
> > > >> This patch effectively restores the behavior prior to svn r34634 if
> > > >> those offending macros are not defined.
> > > >> By this, compilation with older releases of FFmpeg become easier.
> > > >
> > > > That makes no sense, you did not restore the old behaviour. The MPlayer option takes values from 0 to 4 or so, err_recognition takes flags.
> > > > If you want to restore the old behaviour you have to assign error_recognition, but if you do that by checking for the flags you'll break compilation with current Libav.
> > > 
> > > I see. Right, I clearly misunderstood the point of the FF_API_ER deprecation.
> > > 
> > > Variant 3 attached.
> > > 
> > > -- 
> > > regards,
> > >     Reinhard
> > 
> > > Index: libmpcodecs/vd_ffmpeg.c
> > > ===================================================================
> > > --- libmpcodecs/vd_ffmpeg.c	(Revision 34684)
> > > +++ libmpcodecs/vd_ffmpeg.c	(Arbeitskopie)
> > > @@ -266,6 +266,7 @@
> > >      avctx->coded_width = sh->disp_w;
> > >      avctx->coded_height= sh->disp_h;
> > >      avctx->workaround_bugs= lavc_param_workaround_bugs;
> > > +#if defined AV_EF_COMPLIANT && defined AV_EF_CAREFUL && defined AV_EF_AGGRESSIVE
> > >      switch (lavc_param_error_resilience) {
> > >      case 5:
> > >          avctx->err_recognition |= AV_EF_EXPLODE | AV_EF_COMPLIANT | AV_EF_CAREFUL;
> > > @@ -280,6 +281,11 @@
> > >      case 1:
> > >          avctx->err_recognition |= AV_EF_CAREFUL;
> > >      }
> > > +#else
> > > +    /* FF_ER_CAREFUL   (==1) implies AV_EF_CRCCHECK (== 1<<1 - 1),
> > > +       FF_ER_COMPLIANT (==2) implies AV_EF_{CRCCHECK,BITSTREAM} (== 1<<2 - 1), et cetera} */
> > > +    avctx->err_recognition |= (1<<(lavc_param_error_resilience)) - 1;
> > 
> > This is rather obfuscated and depends on internals like the exact values
> > of AV_EF.
> > The bigger problem however is that these flags only go by _type_ of
> > error, however the point of lavc_param_error_resilience is to allow
> > selecting them by _severity_.
> > E.g. AV_EF_BUFFER treats both the MP3 decoder running out of bits
> > and bits being left over as an error, but running out of bits is
> > clearly a far severe problem, the other is most likely just a sloppy
> > encoder.
> > I don't see the point of mapping that option if it can't be
> > mapped to anything compatible with the old meaning.
> > The err_recognition setting is still available via the generic
> > o=... settings that allow setting any option through FFmpeg's
> > option system.
> 
> Hm, I see you probably reacted to Alexander's comments.
> Sorry if you feel pushed back and forth here...
> I don't really consider that option all that important.
> Thus at least erroring out seems a bit extreme, but whatever
> people want...
> I'd be in favour of when it is set print something like
> "... option is not available with your libavcodec version,
> similar functionality is available via o=err_detect=...".

  I am fine with a warning as described above by Reimar. I am
just not OK with silently dropping such options when set by a
user.

  Alexander