Message ID | CAB0OVGoPecu4dLs-O4nkOwJP5Z4svyAnsWgw_Roa9WMTfRi2aA@mail.gmail.com |
---|---|
State | Superseded |
Headers | show |
On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Hi! > > Attached patch fixes the actual output duration for AMR-WB samples > with NO_DATA frames. > A follow-up patch also skips corrupted frames, making the output of > the sample in ticket #7113 very similar to the reference decoder. > > Please comment, Carl Eugen > Very similar does not mean much!
2019-01-28 15:20 GMT+01:00, Paul B Mahol <onemda@gmail.com>: > On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> Hi! >> >> Attached patch fixes the actual output duration for AMR-WB samples >> with NO_DATA frames. >> A follow-up patch also skips corrupted frames, making the output of >> the sample in ticket #7113 very similar to the reference decoder. > > Very similar does not mean much! Since some frames are broken (and not just corrupted) and the codec uses floats internally, I don't think this is relevant. In addition, this patch is not about similarity in the output but duration, so your comment does not apply here. Is this patch ok? Carl Eugen
On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2019-01-28 15:20 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>> Hi! >>> >>> Attached patch fixes the actual output duration for AMR-WB samples >>> with NO_DATA frames. >>> A follow-up patch also skips corrupted frames, making the output of >>> the sample in ticket #7113 very similar to the reference decoder. >> >> Very similar does not mean much! > > Since some frames are broken (and not just corrupted) and the > codec uses floats internally, I don't think this is relevant. > > In addition, this patch is not about similarity in the output but > duration, so your comment does not apply here. > > Is this patch ok? Only if you can confirm that output is same as reference decoder expect rounding. so use 16bit sample format and rounding error should be max -1/+1.
2019-01-28 16:17 GMT+01:00, Paul B Mahol <onemda@gmail.com>: > On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2019-01-28 15:20 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>> Hi! >>>> >>>> Attached patch fixes the actual output duration for AMR-WB samples >>>> with NO_DATA frames. >>>> A follow-up patch also skips corrupted frames, making the output of >>>> the sample in ticket #7113 very similar to the reference decoder. >>> >>> Very similar does not mean much! >> >> Since some frames are broken (and not just corrupted) and the >> codec uses floats internally, I don't think this is relevant. >> >> In addition, this patch is not about similarity in the output but >> duration, so your comment does not apply here. >> >> Is this patch ok? > > Only if you can confirm that output is same as reference decoder > expect rounding. Sorry for the misunderstanding: This patch does not aim to make the output more similar to any other decoder, it only fixes the actual output duration when decoding. Please comment, Carl Eugen
On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2019-01-28 16:17 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>> 2019-01-28 15:20 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>> Hi! >>>>> >>>>> Attached patch fixes the actual output duration for AMR-WB samples >>>>> with NO_DATA frames. >>>>> A follow-up patch also skips corrupted frames, making the output of >>>>> the sample in ticket #7113 very similar to the reference decoder. >>>> >>>> Very similar does not mean much! >>> >>> Since some frames are broken (and not just corrupted) and the >>> codec uses floats internally, I don't think this is relevant. >>> >>> In addition, this patch is not about similarity in the output but >>> duration, so your comment does not apply here. >>> >>> Is this patch ok? >> >> Only if you can confirm that output is same as reference decoder >> expect rounding. > > Sorry for the misunderstanding: > This patch does not aim to make the output more similar to > any other decoder, it only fixes the actual output duration > when decoding. > Than patch is incorrect.
2019-01-28 19:40 GMT+01:00, Paul B Mahol <onemda@gmail.com>: > On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2019-01-28 16:17 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>> 2019-01-28 15:20 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>> Hi! >>>>>> >>>>>> Attached patch fixes the actual output duration for AMR-WB samples >>>>>> with NO_DATA frames. >>>>>> A follow-up patch also skips corrupted frames, making the output of >>>>>> the sample in ticket #7113 very similar to the reference decoder. >>>>> >>>>> Very similar does not mean much! >>>> >>>> Since some frames are broken (and not just corrupted) and the >>>> codec uses floats internally, I don't think this is relevant. >>>> >>>> In addition, this patch is not about similarity in the output but >>>> duration, so your comment does not apply here. >>>> >>>> Is this patch ok? >>> >>> Only if you can confirm that output is same as reference decoder >>> expect rounding. >> >> Sorry for the misunderstanding: >> This patch does not aim to make the output more similar to >> any other decoder, it only fixes the actual output duration >> when decoding. > > Than patch is incorrect. I don't understand: We have a sample that decodes with too short duration with current FFmpeg, the patch fixes this: Why is the patch incorrect? I realize now that by fixing the "missing" parts in the output file, it of course does make the file (significantly) more similar to the reference output - but it does not change the parts of the output that were already there. Carl Eugen
On 1/29/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2019-01-28 19:40 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>> 2019-01-28 16:17 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>> 2019-01-28 15:20 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>>> Hi! >>>>>>> >>>>>>> Attached patch fixes the actual output duration for AMR-WB samples >>>>>>> with NO_DATA frames. >>>>>>> A follow-up patch also skips corrupted frames, making the output of >>>>>>> the sample in ticket #7113 very similar to the reference decoder. >>>>>> >>>>>> Very similar does not mean much! >>>>> >>>>> Since some frames are broken (and not just corrupted) and the >>>>> codec uses floats internally, I don't think this is relevant. >>>>> >>>>> In addition, this patch is not about similarity in the output but >>>>> duration, so your comment does not apply here. >>>>> >>>>> Is this patch ok? >>>> >>>> Only if you can confirm that output is same as reference decoder >>>> expect rounding. >>> >>> Sorry for the misunderstanding: >>> This patch does not aim to make the output more similar to >>> any other decoder, it only fixes the actual output duration >>> when decoding. >> >> Than patch is incorrect. > > I don't understand: > We have a sample that decodes with too short duration with current > FFmpeg, the patch fixes this: Why is the patch incorrect? > > I realize now that by fixing the "missing" parts in the output file, it of > course does make the file (significantly) more similar to the > reference output - but it does not change the parts of the output > that were already there. The patch is incomplete and thus incorrect.
2019-01-29 10:10 GMT+01:00, Paul B Mahol <onemda@gmail.com>: > On 1/29/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2019-01-28 19:40 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>> 2019-01-28 16:17 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>> 2019-01-28 15:20 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>>>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>>>> Hi! >>>>>>>> >>>>>>>> Attached patch fixes the actual output duration for AMR-WB samples >>>>>>>> with NO_DATA frames. >>>>>>>> A follow-up patch also skips corrupted frames, making the output of >>>>>>>> the sample in ticket #7113 very similar to the reference decoder. >>>>>>> >>>>>>> Very similar does not mean much! >>>>>> >>>>>> Since some frames are broken (and not just corrupted) and the >>>>>> codec uses floats internally, I don't think this is relevant. >>>>>> >>>>>> In addition, this patch is not about similarity in the output but >>>>>> duration, so your comment does not apply here. >>>>>> >>>>>> Is this patch ok? >>>>> >>>>> Only if you can confirm that output is same as reference decoder >>>>> expect rounding. >>>> >>>> Sorry for the misunderstanding: >>>> This patch does not aim to make the output more similar to >>>> any other decoder, it only fixes the actual output duration >>>> when decoding. >>> >>> Than patch is incorrect. >> >> I don't understand: >> We have a sample that decodes with too short duration with current >> FFmpeg, the patch fixes this: Why is the patch incorrect? >> >> I realize now that by fixing the "missing" parts in the output file, it of >> course does make the file (significantly) more similar to the >> reference output - but it does not change the parts of the output >> that were already there. > > The patch is incomplete and thus incorrect. Do you mean it does not fix the duration of the NO_DATA frames? Carl Eugen
On 1/29/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2019-01-29 10:10 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >> On 1/29/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>> 2019-01-28 19:40 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>> 2019-01-28 16:17 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>>> 2019-01-28 15:20 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>>>>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>>>>> Hi! >>>>>>>>> >>>>>>>>> Attached patch fixes the actual output duration for AMR-WB samples >>>>>>>>> with NO_DATA frames. >>>>>>>>> A follow-up patch also skips corrupted frames, making the output of >>>>>>>>> the sample in ticket #7113 very similar to the reference decoder. >>>>>>>> >>>>>>>> Very similar does not mean much! >>>>>>> >>>>>>> Since some frames are broken (and not just corrupted) and the >>>>>>> codec uses floats internally, I don't think this is relevant. >>>>>>> >>>>>>> In addition, this patch is not about similarity in the output but >>>>>>> duration, so your comment does not apply here. >>>>>>> >>>>>>> Is this patch ok? >>>>>> >>>>>> Only if you can confirm that output is same as reference decoder >>>>>> expect rounding. >>>>> >>>>> Sorry for the misunderstanding: >>>>> This patch does not aim to make the output more similar to >>>>> any other decoder, it only fixes the actual output duration >>>>> when decoding. >>>> >>>> Than patch is incorrect. >>> >>> I don't understand: >>> We have a sample that decodes with too short duration with current >>> FFmpeg, the patch fixes this: Why is the patch incorrect? >>> >>> I realize now that by fixing the "missing" parts in the output file, it >>> of >>> course does make the file (significantly) more similar to the >>> reference output - but it does not change the parts of the output >>> that were already there. >> >> The patch is incomplete and thus incorrect. > > Do you mean it does not fix the duration of the NO_DATA frames? > Duration is irrelevant. > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
2019-01-29 22:36 GMT+01:00, Paul B Mahol <onemda@gmail.com>: > On 1/29/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2019-01-29 10:10 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>> On 1/29/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>> 2019-01-28 19:40 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>> 2019-01-28 16:17 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>>>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>>>> 2019-01-28 15:20 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>>>>>>> On 1/28/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>>>>>> Hi! >>>>>>>>>> >>>>>>>>>> Attached patch fixes the actual output duration for AMR-WB samples >>>>>>>>>> with NO_DATA frames. >>>>>>>>>> A follow-up patch also skips corrupted frames, making the output >>>>>>>>>> of >>>>>>>>>> the sample in ticket #7113 very similar to the reference decoder. >>>>>>>>> >>>>>>>>> Very similar does not mean much! >>>>>>>> >>>>>>>> Since some frames are broken (and not just corrupted) and the >>>>>>>> codec uses floats internally, I don't think this is relevant. >>>>>>>> >>>>>>>> In addition, this patch is not about similarity in the output but >>>>>>>> duration, so your comment does not apply here. >>>>>>>> >>>>>>>> Is this patch ok? >>>>>>> >>>>>>> Only if you can confirm that output is same as reference decoder >>>>>>> expect rounding. >>>>>> >>>>>> Sorry for the misunderstanding: >>>>>> This patch does not aim to make the output more similar to >>>>>> any other decoder, it only fixes the actual output duration >>>>>> when decoding. >>>>> >>>>> Than patch is incorrect. >>>> >>>> I don't understand: >>>> We have a sample that decodes with too short duration with current >>>> FFmpeg, the patch fixes this: Why is the patch incorrect? >>>> >>>> I realize now that by fixing the "missing" parts in the output file, it >>>> of >>>> course does make the file (significantly) more similar to the >>>> reference output - but it does not change the parts of the output >>>> that were already there. >>> >>> The patch is incomplete and thus incorrect. >> >> Do you mean it does not fix the duration of the NO_DATA frames? >> > > Duration is irrelevant. Not sure I understand: This is about 4 seconds vs 16 seconds... Anyway, I will send a new patch that initializes the output frame, this improves the result. Carl Eugen
From ab817b2c8fc96ddabc8238c9be2beed04cd54512 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Mon, 28 Jan 2019 15:10:13 +0100 Subject: [PATCH] lavc/amrwbdec: Do not ignore NO_DATA frames. Fixes the actual output duration of the sample in ticket #7113. --- libavcodec/amrwbdata.h | 2 +- libavcodec/amrwbdec.c | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/libavcodec/amrwbdata.h b/libavcodec/amrwbdata.h index 8a8cbfd..95c0aaa 100644 --- a/libavcodec/amrwbdata.h +++ b/libavcodec/amrwbdata.h @@ -1884,7 +1884,7 @@ static const float lpf_7_coef[31] = { // low pass, 7kHz cutoff /** Core frame sizes in each mode */ static const uint16_t cf_sizes_wb[] = { 132, 177, 253, 285, 317, 365, 397, 461, 477, - 40 /// SID/comfort noise frame + 40, 0, 0, 0, 0, 0, 0 }; #endif /* AVCODEC_AMRWBDATA_H */ diff --git a/libavcodec/amrwbdec.c b/libavcodec/amrwbdec.c index 47fe7eb..997b3e8 100644 --- a/libavcodec/amrwbdec.c +++ b/libavcodec/amrwbdec.c @@ -1119,12 +1119,17 @@ static int amrwb_decode_frame(AVCodecContext *avctx, void *data, buf_out = (float *)frame->data[0]; header_size = decode_mime_header(ctx, buf); + expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7) >> 3) + 1; + + if (ctx->fr_cur_mode == NO_DATA) { + *got_frame_ptr = 1; + return expected_fr_size; + } if (ctx->fr_cur_mode > MODE_SID) { av_log(avctx, AV_LOG_ERROR, "Invalid mode %d\n", ctx->fr_cur_mode); return AVERROR_INVALIDDATA; } - expected_fr_size = ((cf_sizes_wb[ctx->fr_cur_mode] + 7) >> 3) + 1; if (buf_size < expected_fr_size) { av_log(avctx, AV_LOG_ERROR, -- 1.7.10.4