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

[Sun Zhenliang]

在 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.
> > >
> > > >
> > > > - sunzhenliang
> > > > > > - sunzhenliang
> > > > > >
> > > > > >
> > > > > >
> > > > > > _______________________________________________
> > > > > > ffmpeg-devel mailing list
> > > > > > ffmpeg-devel at ffmpeg.org
> > > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > >
> > > > > > To unsubscribe, visit link above, or email
> > > > > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> > > > > >
> > > > > _______________________________________________
> > > > > ffmpeg-devel mailing list
> > > > > ffmpeg-devel at ffmpeg.org
> > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > >
> > > > > To unsubscribe, visit link above, or email
> > > > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel at ffmpeg.org
> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >
> > > > To unsubscribe, visit link above, or email
> > > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> > > >
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel at ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".