Opened 4 years ago

Closed 4 years ago

#1351 closed defect (fixed)

mpeg2video lowres support

Reported by: jyavenard Owned by:
Priority: important Component: avcodec
Version: git-master Keywords: mpeg2video regression lowres
Cc: Blocked By:
Blocking: Reproduced by developer: yes
Analyzed by developer: no

Description

In:

commit 2bcbd98459915baefc15043d02f4a942ebcd33da
Author: Mans Rullgard <mans@mansr.com>
Date:   Thu Apr 12 13:55:49 2012 +0100

    Remove lowres video decoding
    
    This feature is complex, of questionable utility, and slows down
    normal decoding.
    
    Signed-off-by: Mans Rullgard <mans@mansr.com>

low-res video decoding is used in mythtv in order to flag a video for commercial.

Removing this mode breaks mythtv's comm flagging.
Doing comm flagging without low res reduced speed by a factor of two.

So I would like to plead to reverse that change.
There was no need to remove it.

Thank you

Change History (13)

comment:1 Changed 4 years ago by cehoyos

Afaict, current FFmpeg does support lowres...

comment:2 Changed 4 years ago by jyavenard

Not this the merge in 92ef4be4ab9fbb7d901b22e0036a4ca90b00a476

Do you really think I would have lodged this issue if that wasn't the case?

commit 92ef4be4ab9fbb7d901b22e0036a4ca90b00a476
Merge: 2e07f42 d526c53
Author: Michael Niedermayer <michaelni@gmx.at>
Date:   Sun Apr 22 22:26:42 2012 +0200

    Merge remote-tracking branch 'qatar/master'
    
    * qatar/master:
      ARM: allow runtime masking of CPU features
      dsputil: remove unused functions
      mov: Treat keyframe indexes as 1-origin if starting at non-zero.
      mov: Take stps entries into consideration also about key_off.
      Remove lowres video decoding

which included the commit in the original report

Version 0, edited 4 years ago by jyavenard (next)

comment:3 Changed 4 years ago by cehoyos

  • Component changed from undetermined to avcodec
  • Keywords mpeg2video regression added
  • Priority changed from normal to important
  • Reproduced by developer set
  • Status changed from new to open
  • Summary changed from Removal of mpeg2 low-res video decoding: why? to mpeg2video lowres support
  • Version changed from unspecified to git-master

The relevant commit is 5e50a57

comment:4 Changed 4 years ago by cehoyos

  • Resolution set to fixed
  • Status changed from open to closed

This appears to have been fixed, thank you for your report!

comment:5 Changed 4 years ago by cehoyos

  • Keywords lowres added

comment:6 follow-up: Changed 4 years ago by jyavenard

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thank you for this...

However, are you sure the revert is complete?

The git comment itself is wrong, and the sha mentioned isn't the correct one:
http://git.videolan.org/?p=ffmpeg.git;a=commit;h=5e50a5724bb00c44a98bd89f57c659a613f26ce2

The merged commit (from Libav) that removed lowres was
2bcbd98459915baefc15043d02f4a942ebcd33da
http://git.videolan.org/?p=ffmpeg.git;a=commit;h=2bcbd98459915baefc15043d02f4a942ebcd33da

it modified the files:

avconv.c
avplay.c
libavcodec/alpha/dsputil_alpha.c
libavcodec/arm/dsputil_init_arm.c
libavcodec/arm/dsputil_init_armv5te.c
libavcodec/arm/dsputil_init_armv6.c
libavcodec/arm/dsputil_init_neon.c
libavcodec/avcodec.h
libavcodec/dsputil.c
libavcodec/dsputil.h
libavcodec/dv.c
libavcodec/dvdec.c
libavcodec/error_resilience.c
libavcodec/flvdec.c
libavcodec/h261dec.c
libavcodec/h263dec.c
libavcodec/intrax8.c
libavcodec/jrevdct.c
libavcodec/libopenjpeg.c
libavcodec/mjpegbdec.c
libavcodec/mjpegdec.c
libavcodec/mpeg12.c
libavcodec/mpeg4videodec.c
libavcodec/mpegvideo.c
libavcodec/mpegvideo.h
libavcodec/msmpeg4.c
libavcodec/mxpegdec.c
libavcodec/options_table.h
libavcodec/ppc/dsputil_ppc.c
libavcodec/rv10.c
libavcodec/sp5xdec.c
libavcodec/utils.c
libavcodec/wmv2dec.c
libavcodec/x86/dsputil_mmx.c

Also, a following commits removed DSP code that were used for lowres support:
d7458bc8c62ae1cb2ffc805b989fcddf4029dda6
http://git.videolan.org/?p=ffmpeg.git;a=commit;h=d7458bc8c62ae1cb2ffc805b989fcddf4029dda6

So the reversal made is only partial and only going to have an effect on mpeg decoder...

BTW, I agree, there's no way four extra bit shifts would have caused a drop of speed of 3%... Those stats could have only been made up...

comment:7 in reply to: ↑ 6 Changed 4 years ago by cehoyos

Replying to jyavenard:

However, are you sure the revert is complete?

No.

You reported that mpeg2video lowres support is missing (I originally read only the body of the ticket, where it only says "lowres was removed", I did not understand the problem because lowres did exist at least for mpeg4 and dvvideo at the time you reported the problem).
Since yesterday, mpeg2video lowres works fine here.

If you believe there still is an issue, please provide a failing command line together with complete, uncut output (you should have done this originally).

comment:8 follow-ups: Changed 4 years ago by jyavenard

Command line of what? this is a libavcodec issue and about features dropped from it.

yes, mpeg2video lowres support was re-enabled. But what about lowres support for all the other codecs that were removed in that same commit.
Those were not reverted, i have listed above all the codecs that suddenly saw their lowres support dropped.

comment:9 in reply to: ↑ 8 Changed 4 years ago by cehoyos

Replying to jyavenard:

yes, mpeg2video lowres support was re-enabled. But what about lowres support for all the other codecs that were removed in that same commit.

If you add a failing command line, it would be easier to understand which codecs you are talking about, I could then close this ticket (that you opened about mpeg2video) and open a new one.

You are right that tickets against libavcodec can be opened, but long time experience tells us that tickets that are reproducible with ffmpeg are much faster fixed.

comment:10 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by michael

Note, the lowres removial was reverted in 2 commits, first was 70d54392f5015b9c6594fcae558f59f952501e3b.
Ive not double checked now but i belive that the 2 commits brought all parts back.

comment:11 in reply to: ↑ 10 Changed 4 years ago by cehoyos

Replying to michael:

Ive not double checked now but i belive that the 2 commits brought all parts back.

I believe wmv2 is still missing, I don't know if this is more difficult / problematic because of IntraX8 / J-frames.

comment:12 Changed 4 years ago by jyavenard

Thanks Michael.. I see what went on...
I was confused with the name of "lowres2" and skipped that commit

So it was just mpeg2 that didn't get revert properly at first...

Excellent news..

I can tell you for sure now that MythTV will sync with ffmpeg from now on.

comment:13 Changed 4 years ago by cehoyos

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.