Message ID | 20180429191918.2915-3-michael@niedermayer.cc |
---|---|
State | Accepted |
Commit | 9f73ae31e075104c7613d481a09a8b102e6449e9 |
Headers | show |
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 [...]
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;
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/mpeg4videodec.c | 4 ---- 1 file changed, 4 deletions(-)