[FFmpeg-devel] [PATCH v1 1/1] libavformat/amr.c: remove mode range check

Tue Sep 14 04:36:15 EEST 2021

在 2021年9月8日 +0800 22:12，Sun Zhenliang <hisunzhenliang at outlook.com>，写道：
> 在 2021年9月8日 +0800 17:10，Paul B Mahol <onemda at gmail.com>，写道：
> > On Wed, Sep 8, 2021 at 10:56 AM Sun Zhenliang <hisunzhenliang at outlook.com>
> > wrote:
> >
> > > 在 2021年9月8日 +0800 16:37，Paul B Mahol <onemda at gmail.com>，写道：
> > > > On Wed, Sep 8, 2021 at 4:23 AM Sun Zhenliang <hisunzhenliang at outlook.com
> > > >
> > > > wrote:
> > > >
> > > > > 在 2021年9月8日 +0800 00:14，Paul B Mahol <onemda at gmail.com>，写道：
> > > > > > On Tue, Sep 7, 2021 at 4:48 PM Sun Zhenliang <
> > > hisunzhenliang at outlook.com
> > > > > >
> > > > > > wrote:
> > > > > >
> > > > > > > 在 2021年9月7日 +0800 19:13，Paul B Mahol <onemda at gmail.com>，写道：
> > > > > > > > On Tue, Sep 7, 2021 at 11:36 AM Sun Zhenliang <
> > > > > > > hisunzhenliang at outlook.com>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > 在 2021年9月7日 +0800 14:16，Paul B Mahol <onemda at gmail.com>，写道：
> > > > > > > > > > On Tue, Sep 7, 2021 at 7:34 AM Sun Zhenliang <
> > > > > > > hisunzhenliang at outlook.com
> > > > > > > > > >
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > 在 2021年9月7日 +0800 00:55，Paul B Mahol <onemda at gmail.com
> > > > ，写道：
> > > > > > > > > > > > On Sat, Sep 4, 2021 at 1:52 PM Sun Zhenliang <
> > > > > > > > > hisunzhenliang at outlook.com
> > > > > > > > > > > >
> > > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Sep 4, 2021, at 16:01, Hendrik Leppkes <
> > > > > > > h.leppkes at gmail.com>
> > > > > > > > > > > wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Sat, Sep 4, 2021 at 9:10 AM sunzhenliang <
> > > > > > > > > > > hisunzhenliang at outlook.com>
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Those comfort noise frames and empty frames should
> > > be
> > > > > > > > > > > > > > > considered the correct frame. And
> > > > > amr.c/amr_read_packet()
> > > > > > > > > > > > > > > also takes them as correct frames too.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: sunzhenliang <
> > > > > hisunzhenliang at outlook.com>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > libavformat/amr.c | 4 ++--
> > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > diff --git a/libavformat/amr.c b/libavformat/amr.c
> > > > > > > > > > > > > > > index 836b276fd5..540d8033d8 100644
> > > > > > > > > > > > > > > --- a/libavformat/amr.c
> > > > > > > > > > > > > > > +++ b/libavformat/amr.c
> > > > > > > > > > > > > > > @@ -178,7 +178,7 @@ static int amrnb_probe(const
> > > > > > > AVProbeData
> > > > > > > > > *p)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > while (i < p->buf_size) {
> > > > > > > > > > > > > > > mode = b[i] >> 3 & 0x0F;
> > > > > > > > > > > > > > > - if (mode < 9 && (b[i] & 0x4) == 0x4) {
> > > > > > > > > > > > > > > + if ((b[i] & 0x4) == 0x4) {
> > > > > > > > > > > > > > > int last = b[i];
> > > > > > > > > > > > > > > int size = amrnb_packed_size[mode];
> > > > > > > > > > > > > > > while (size--) {
> > > > > > > > > > > > > > > @@ -234,7 +234,7 @@ static int amrwb_probe(const
> > > > > > > AVProbeData
> > > > > > > > > *p)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > while (i < p->buf_size) {
> > > > > > > > > > > > > > > mode = b[i] >> 3 & 0x0F;
> > > > > > > > > > > > > > > - if (mode < 10 && (b[i] & 0x4) == 0x4) {
> > > > > > > > > > > > > > > + if ((b[i] & 0x4) == 0x4) {
> > > > > > > > > > > > > > > int last = b[i];
> > > > > > > > > > > > > > > int size = amrwb_packed_size[mode];
> > > > > > > > > > > > > > > while (size--) {
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This is just probing, and expecting to find a
> > > significant
> > > > > > > amount
> > > > > > > > > of
> > > > > > > > > > > > > > non-empty/non-noise frames seems like something you
> > > would
> > > > > > > want in
> > > > > > > > > > > > > > probing.
> > > > > > > > > > > > > > Does this actually fix detection of any samples?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > It seems like this has the potential of quite
> > > > > substantially
> > > > > > > > > reducing
> > > > > > > > > > > > > > the number of checked bits and thus invalid
> > > detections.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > - Hendrik
> > > > > > > > > > > > > > ____________________________________________
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks for reviewing.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Probing is expecting to find “correct” frames, which
> > > > > includes
> > > > > > > those
> > > > > > > > > > > kinds
> > > > > > > > > > > > > of comfort noise and NO_DATA frames mentioned in 3GPP
> > > > > > > > > Specifications.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Besides, those frames are considered as normal frames
> > > while
> > > > > > > reading
> > > > > > > > > > > > > packets. It seems better to take the same standard of
> > > > > frames in
> > > > > > > > > both
> > > > > > > > > > > > > reading and probing.
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > Yes, but in past IIRC it caused misdetection when
> > > probing.
> > > > > > > > > > > But it may cause misdetection when probing audios with many
> > > > > > > > > > > comfort noise and NO_DATA frames. Maybe we should stick to
> > > > > > > > > > > 3GPP Specifications.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > And at same time not broke probing for more more important
> > > > > formats.
> > > > > > > > > > Have you checked that your patch does not cause regressions?
> > > > > > > > > >
> > > > > > > > > I have done fate-test, but can not submit the results with
> > > > > fate_recv.
> > > > > > > > > I have sent the public ssh key to fate-admin at ffmpeg.org,
> > > maybe it
> > > > > > > > > needs some time?
> > > > > > > > >
> > > > > > > >
> > > > > > > > I mean regressions not covered by FATE.
> > > > > > > >
> > > > > > > > Also you misunderstood fate-test, this patch have nothing to do
> > > with
> > > > > > > > sending your fate testing reports to FFmpeg.
> > > > > > > >
> > > > > > > Sorry, could you explain more in details? I am confused.
> > > > > > >
> > > > > > > I have checked this patch, which works fine with those amr files.
> > > > > > >
> > > > > >
> > > > > > What about non-amr files mentioned in tickets in git log history of
> > > this
> > > > > > demuxer.
> > > > > >
> > > > > Thanks for explaining, I understand now.
> > > > > I have checked those tickets in git history of this demuxer
> > > > > (tickets #7270, #7125, #6208, #3541).
> > > > > It works fine.
> > > > >
> > > >
> > > > So you actually compiled ffmpeg with midi support at tested that those
> > > midi
> > > > files are working fine?
> > > >
> > > >
> > > No, I just tested those midi file added in #7270, which were not
> > > misdetected as amr.
> > >
> >
> > Sorry, That means you have not tested that at all.
> >
> >
> Sorry, I am not familiar with midi. Is it ok to just compile ffmpeg with libmodplug?
> because I just find .mid is defined in libmodplug.c.
>
> Here is the ffmpeg log to test midi file in #7270.
>
>   ~/Downloads ❯ ffmpeg -i not_amr.mid -loglevel 48
> ffmpeg version N-103542-gb9426f371a Copyright (c) 2000-2021 the FFmpeg developers
>  built with gcc 9 (Ubuntu 9.3.0-17ubuntu1~20.04)
>  configuration: --prefix=/home/sun/.local --enable-shared --enable-sdl --enable-gpl
> --enable-version3 --enable-libmp3lame --enable-debug --enable-libopencore-amrnb
> --enable-libopencore-amrwb --enable-libvo-amrwbenc --enable-libx265 --enable-libx264
> --disable-x86asm --enable-libmodplug
>  libavutil 57. 4.101 / 57. 4.101
>  libavcodec 59. 7.101 / 59. 7.101
>  libavformat 59. 5.100 / 59. 5.100
>  libavdevice 59. 0.101 / 59. 0.101
>  libavfilter 8. 7.101 / 8. 7.101
>  libswscale 6. 1.100 / 6. 1.100
>  libswresample 4. 0.100 / 4. 0.100
>  libpostproc 56. 0.100 / 56. 0.100
> Splitting the commandline.
> Reading option '-i' ... matched as input url with argument 'not_amr.mid'.
> Reading option '-loglevel' ... matched as option 'loglevel' (set logging level) with argument '48'.
> Finished splitting the commandline.
> Parsing a group of options: global .
> Applying option loglevel (set logging level) with argument 48.
> Successfully parsed a group of options.
> Parsing a group of options: input url not_amr.mid.
> Successfully parsed a group of options.
> Opening an input file: not_amr.mid.
> [NULL @ 0x55bafaa57e40] Opening 'not_amr.mid' for reading
> [file @ 0x55bafaa58780] Setting default whitelist 'file,crypto,data'
> [AVIOContext @ 0x55bafaa60b40] Statistics: 4435 bytes read, 0 seeks
> not_amr.mid: Invalid data found when processing input
>
> Is it ok?
> > > > > > > > > >
> > > > > > > > > > >
Ping