diff mbox

[FFmpeg-devel] lavc/amrwbdec: Do not ignore NO_DATA frames

Message ID CAB0OVGoPecu4dLs-O4nkOwJP5Z4svyAnsWgw_Roa9WMTfRi2aA@mail.gmail.com
State Superseded
Headers show

Commit Message

Carl Eugen Hoyos Jan. 28, 2019, 2:13 p.m. UTC 
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

Comments

Paul B Mahol Jan. 28, 2019, 2:20 p.m. UTC | #1 
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!
Carl Eugen Hoyos Jan. 28, 2019, 2:34 p.m. UTC | #2 
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
Paul B Mahol Jan. 28, 2019, 3:17 p.m. UTC | #3 
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.
Carl Eugen Hoyos Jan. 28, 2019, 4:03 p.m. UTC | #4 
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
Paul B Mahol Jan. 28, 2019, 6:40 p.m. UTC | #5 
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.
Carl Eugen Hoyos Jan. 29, 2019, 4:17 a.m. UTC | #6 
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
Paul B Mahol Jan. 29, 2019, 9:10 a.m. UTC | #7 
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.
Carl Eugen Hoyos Jan. 29, 2019, 9:15 p.m. UTC | #8 
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
Paul B Mahol Jan. 29, 2019, 9:36 p.m. UTC | #9 
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
>
Carl Eugen Hoyos Jan. 29, 2019, 9:45 p.m. UTC | #10 
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
diff mbox

Patch

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