[FFmpeg-devel,3/5] avcodec/mpeg4videode: Eliminate out of loop VOP startcode reading for studio profile

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/mpeg4videodec.c | 4 ----
 1 file changed, 4 deletions(-)

On Sun, 29 Apr 2018 at 20:20 Michael Niedermayer <michael@niedermayer.cc>
wrote:

> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/mpeg4videodec.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
> index 9ee2f37c69..9318d7bd4e 100644
> --- a/libavcodec/mpeg4videodec.c
> +++ b/libavcodec/mpeg4videodec.c
> @@ -2931,9 +2931,6 @@ static int decode_studio_vop_header(Mpeg4DecContext
> *ctx, GetBitContext *gb)
>      if (get_bits_left(gb) <= 32)
>          return 0;
>
> -    if (get_bits_long(gb, 32) != VOP_STARTCODE)
> -        return AVERROR_INVALIDDATA;
> -
>      s->decode_mb = mpeg4_decode_studio_mb;
>
>      decode_smpte_tc(ctx, gb);
> @@ -3183,7 +3180,6 @@ int ff_mpeg4_decode_picture_header(Mpeg4DecContext
> *ctx, GetBitContext *gb)
>              if (s->studio_profile) {
>                  if ((ret = decode_studio_vol_header(ctx, gb)) < 0)
>                      return ret;
> -                break;
>              } else {
>                  if ((ret = decode_vol_header(ctx, gb)) < 0)
>                      return ret;
> --
> 2.17.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


I appreciate you are going to ram this through as "maintainer" but many of
these changes do not reflect the intentions of the specification and build
on bad design decisions in mpeg4videodec from 10-15 years ago.

Kieran

On Sun, Apr 29, 2018 at 11:24:19PM +0000, Kieran Kunhya wrote:
> On Sun, 29 Apr 2018 at 20:20 Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/mpeg4videodec.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
> > index 9ee2f37c69..9318d7bd4e 100644
> > --- a/libavcodec/mpeg4videodec.c
> > +++ b/libavcodec/mpeg4videodec.c
> > @@ -2931,9 +2931,6 @@ static int decode_studio_vop_header(Mpeg4DecContext
> > *ctx, GetBitContext *gb)
> >      if (get_bits_left(gb) <= 32)
> >          return 0;
> >
> > -    if (get_bits_long(gb, 32) != VOP_STARTCODE)
> > -        return AVERROR_INVALIDDATA;
> > -
> >      s->decode_mb = mpeg4_decode_studio_mb;
> >
> >      decode_smpte_tc(ctx, gb);
> > @@ -3183,7 +3180,6 @@ int ff_mpeg4_decode_picture_header(Mpeg4DecContext
> > *ctx, GetBitContext *gb)
> >              if (s->studio_profile) {
> >                  if ((ret = decode_studio_vol_header(ctx, gb)) < 0)
> >                      return ret;
> > -                break;
> >              } else {
> >                  if ((ret = decode_vol_header(ctx, gb)) < 0)
> >                      return ret;
> > --
> > 2.17.0
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 
> I appreciate you are going to ram this through as "maintainer" but many of
> these changes do not reflect the intentions of the specification and build
> on bad design decisions in mpeg4videodec from 10-15 years ago.

Id like to move forward, the rest of this patchset as well as the remaining
patch of the other set depend on this.

If you have technical arguments iam interrested to hear them. I do not share
your view though.
The specification, or rather the authors of the specification did not had the
intend that every line would be 1:1 copied into an implementation. I think this
is not even done in the standard comitees reference implementations in general.

About the past, it appears to me that there is a deeply rooted aversion by
some people toward some code. This just doesnt belong here.

The parsing of a format or codec by a main loop and a "switch" on the "chunk"
type is very common. This style is used in modern code as well. And probably
was in many places used as a response towards to practical problems with 
more "spec like" rigid parsing. At least for avi and wav i remember that
applications using strict parsing had issues playing some files ...

About this here more specifically, the old code can handle basically every
mpeg4 file. The new studio_profile code fails to play every of the reference
streams, it does only play the 3 non reference streams we have

So if there are no objections i will in a day or 2 apply this and the rest
of the patchset(s)

Thanks

[...]

>
> About the past, it appears to me that there is a deeply rooted aversion by
> some people toward some code. This just doesnt belong here.
>

Correct, mpeg4video decoding is some of the least understandable and
undocumented code in the entirety of libavcodec.
Decoding spans the following files in a completely crazy way:

mpeg4videodec.c
h263dec.c
ituh263dec.c
mpegvideo.c

Lots and lots of undocumented heuristics:
https://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/h263dec.c;h=eae29fa438e3b6b5adadbbe5d94363e2303ef7ee;hb=HEAD#l321
etc.

I appreciate I have the benefit of hindsight but for example with DPCM
there's no way to implement it without doing a block memcpy because of the
weird abstraction.

Kieran

On Thu, May 03, 2018 at 12:58:56AM +0000, Kieran Kunhya wrote:
> >
> > About the past, it appears to me that there is a deeply rooted aversion by
> > some people toward some code. This just doesnt belong here.
> >
> 
> Correct, mpeg4video decoding is some of the least understandable and
> undocumented code in the entirety of libavcodec.
> Decoding spans the following files in a completely crazy way:
> 
> mpeg4videodec.c
> h263dec.c
> ituh263dec.c
> mpegvideo.c
> 

> Lots and lots of undocumented heuristics:
> https://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/h263dec.c;h=eae29fa438e3b6b5adadbbe5d94363e2303ef7ee;hb=HEAD#l321
> etc.

Can you post a list of these ? (not just "etc")
ill try to look over them and add documentation

thanks

[...]