diff mbox series

[FFmpeg-devel] avcodec/dolby_e: set constant frame_size

Message ID 20201215171324.99-1-nicolas.gaullier@cji.paris
State New
Headers show
Series [FFmpeg-devel] avcodec/dolby_e: set constant frame_size
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Nicolas Gaullier Dec. 15, 2020, 5:13 p.m. UTC
Fixes pts generation.

Setting frame_size in dolby_e_init() or get_audio_frame_duration()
can result in a bad duration value for the first packet if dolby_e is
muxed in a container having a different sample_rate (ex:
container @48KHz, DolbyE @44.8KHz).
Maybe adding a parser to dolby_e would fix the issue and makes it
possible to set frame_size at decoder init which seems the best place.
---
 libavcodec/dolby_e.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Nicolas Gaullier Jan. 4, 2021, 12:39 p.m. UTC | #1
>De : Nicolas Gaullier <nicolas.gaullier@cji.paris> 
>Envoyé : mardi 15 décembre 2020 18:13
>À : ffmpeg-devel@ffmpeg.org
>Cc : Nicolas Gaullier <nicolas.gaullier@cji.paris>
>Objet : [PATCH] avcodec/dolby_e: set constant frame_size
>
>Fixes pts generation.
>
>Setting frame_size in dolby_e_init() or get_audio_frame_duration() can result in a bad duration value for the first packet if dolby_e is muxed in a container having a different sample_rate (ex:
>container @48KHz, DolbyE @44.8KHz).
>Maybe adding a parser to dolby_e would fix the issue and makes it possible to set frame_size at decoder init which seems the best place.
>---
> libavcodec/dolby_e.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/libavcodec/dolby_e.c b/libavcodec/dolby_e.c index 429612ec08..b0e6d6aee3 100644
>--- a/libavcodec/dolby_e.c
>+++ b/libavcodec/dolby_e.c
>@@ -577,6 +577,7 @@ static int filter_frame(DBEContext *s, AVFrame *frame)
>         reorder = ch_reorder_n;
> 
>     frame->nb_samples = FRAME_SAMPLES;
>+    s->avctx->frame_size = FRAME_SAMPLES;
>     if ((ret = ff_get_buffer(s->avctx, frame, 0)) < 0)
>         return ret;
> 
>--
>2.27.0.windows.1

Hello,
Just a ping for this little patch
Thx
Nicolas
Marton Balint Jan. 4, 2021, 8:32 p.m. UTC | #2
On Mon, 4 Jan 2021, Nicolas Gaullier wrote:

>> De : Nicolas Gaullier <nicolas.gaullier@cji.paris> 
>> Envoyé : mardi 15 décembre 2020 18:13
>> À : ffmpeg-devel@ffmpeg.org
>> Cc : Nicolas Gaullier <nicolas.gaullier@cji.paris>
>> Objet : [PATCH] avcodec/dolby_e: set constant frame_size
>>
>> Fixes pts generation.
>>
>> Setting frame_size in dolby_e_init() or get_audio_frame_duration() can result in a bad duration value for the first packet if dolby_e is muxed in a container having a different sample_rate (ex:
>> container @48KHz, DolbyE @44.8KHz).
>> Maybe adding a parser to dolby_e would fix the issue and makes it possible to set frame_size at decoder init which seems the best place.

I am not sure I understand this. It is suprising that you say that 
frame_size cannot be set in dolby_e_init(), why does it matter? It can 
only be FRAME_SAMPLES, no other values can happen. In that sense it is 
similar to sample_fmt, which I also don't see why it is set in every 
decode call, and not only once, in init.

>> ---
>> libavcodec/dolby_e.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/libavcodec/dolby_e.c b/libavcodec/dolby_e.c index 429612ec08..b0e6d6aee3 100644
>> --- a/libavcodec/dolby_e.c
>> +++ b/libavcodec/dolby_e.c
>> @@ -577,6 +577,7 @@ static int filter_frame(DBEContext *s, AVFrame *frame)
>>         reorder = ch_reorder_n;
>>
>>     frame->nb_samples = FRAME_SAMPLES;
>> +    s->avctx->frame_size = FRAME_SAMPLES;

If you still believe that setting this is required in every decode call, 
then I'd say it would be cleaner to set this at dolby_e_decode_frame where 
other avctx parameters are also set.

Thanks,
Marton

>>     if ((ret = ff_get_buffer(s->avctx, frame, 0)) < 0)
>>         return ret;
>> 
>> --
>> 2.27.0.windows.1
>
> Hello,
> Just a ping for this little patch
> Thx
> Nicolas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas Gaullier Jan. 5, 2021, 9:43 a.m. UTC | #3
>>> De : Nicolas Gaullier <nicolas.gaullier@cji.paris> Envoyé : mardi 15 
>>> décembre 2020 18:13 À : ffmpeg-devel@ffmpeg.org Cc : Nicolas Gaullier 
>>> <nicolas.gaullier@cji.paris> Objet : [PATCH] avcodec/dolby_e: set 
>>> constant frame_size
>>>
>>> Fixes pts generation.
>>>
>>> Setting frame_size in dolby_e_init() or get_audio_frame_duration() can result in a bad duration value for the first packet if dolby_e is muxed in a container having a different sample_rate (ex:
>>> container @48KHz, DolbyE @44.8KHz).
>>> Maybe adding a parser to dolby_e would fix the issue and makes it possible to set frame_size at decoder init which seems the best place.
>
>I am not sure I understand this. It is suprising that you say that frame_size cannot be set in dolby_e_init(), why does it matter? It can only be FRAME_SAMPLES, no other values can happen. In that sense it is similar to sample_fmt, which I also don't see why it is set in every decode call, and not only once, in init.

Yes, this is not easy to see because the current code does not make this problem show up, but my initial target is a patch serie to support dolby_e in the wav container, and it makes it very clear.
Please look at :
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20201019081929.1926-9-nicolas.gaullier@cji.paris/
The issue is that the WAV container has typically a sample rate of 48000Hz which contains the s377m submux that embedd a 44800Hz stream ("pal"-DolbyE here) : all this is very typical, standard, DolbyE content, but there is a trick in compute_pkt_fields (if I remember correctly my testing) with early duration setting based on 48000Hz which results in a wrong value for the first frame.
Maybe having a DolbyE parser would fix this (the 44800Hz would araise sooner), but currently the pts are broken.

Here is the diff if I set frame_size at dolby_e_init:
--- ./tests/ref/fate/s337m-wav  2020-12-15 18:02:28.166747900 +0100
+++ tests/data/fate/s337m-wav   2021-01-05 10:27:01.193976500 +0100
@@ -4,8 +4,8 @@
 #sample_rate 0: 44800
 #channel_layout 0: 63f
 #channel_layout_name 0: 7.1
-0,          0,          0,     1920,    11496, 0x05a9c147
-0,       1920,       1920,     1920,    11496, 0x1d44d2b4
-0,       3840,       3840,     1920,    11496, 0x4e078953
-0,       5760,       5760,     1920,    11496, 0x1c73b1a1
-0,       7680,       7680,     1920,    11262, 0xfa179fc8
+0,          0,          0,     1792,    11496, 0x05a9c147
+0,       1792,       1792,     1920,    11496, 0x1d44d2b4
+0,       3712,       3712,     1920,    11496, 0x4e078953
+0,       5632,       5632,     1920,    11496, 0x1c73b1a1
+0,       7552,       7552,     1920,    11262, 0xfa179fc8

>>> ---
>>> libavcodec/dolby_e.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/libavcodec/dolby_e.c b/libavcodec/dolby_e.c index 
>>> 429612ec08..b0e6d6aee3 100644
>>> --- a/libavcodec/dolby_e.c
>>> +++ b/libavcodec/dolby_e.c
>>> @@ -577,6 +577,7 @@ static int filter_frame(DBEContext *s, AVFrame *frame)
>>>         reorder = ch_reorder_n;
>>>
>>>     frame->nb_samples = FRAME_SAMPLES;
>>> +    s->avctx->frame_size = FRAME_SAMPLES;
>
>If you still believe that setting this is required in every decode call, then I'd say it would be cleaner to set this at dolby_e_decode_frame where other avctx parameters are also set.
>
>Thanks,
>Marton

I agree with you that sample_fmt and frame_size are both "const" and should probably be set at the same place wherever it is, and preferably at dolby_e_init.
I cannot set frame_size in dolby_e_init because of the trick and sample_fmt is already set at dolby_e_decode_frame (for an unknown reason), maybe I should set frame_size in dolby_e_decode_frame too.

I have just tested setting frame_size at dolby_e_decode_frame, and I confirm : yes, it works.
This is not ideal but in the very short term, I really cannot see any other option : will you approve the patch if I set frame_size at dolby_e_decode_frame instead of filter_frame ? Should I amend my commit msg?

Thanks to you
Nicolas
Lynne Jan. 5, 2021, 3:25 p.m. UTC | #4
Jan 5, 2021, 10:43 by nicolas.gaullier@cji.paris:

>>>> De : Nicolas Gaullier <nicolas.gaullier@cji.paris> Envoyé : mardi 15 
>>>> décembre 2020 18:13 À : ffmpeg-devel@ffmpeg.org Cc : Nicolas Gaullier 
>>>> <nicolas.gaullier@cji.paris> Objet : [PATCH] avcodec/dolby_e: set 
>>>> constant frame_size
>>>>
>>>> Fixes pts generation.
>>>>
>>>> Setting frame_size in dolby_e_init() or get_audio_frame_duration() can result in a bad duration value for the first packet if dolby_e is muxed in a container having a different sample_rate (ex:
>>>> container @48KHz, DolbyE @44.8KHz).
>>>> Maybe adding a parser to dolby_e would fix the issue and makes it possible to set frame_size at decoder init which seems the best place.
>>>>
> >I am not sure I understand this. It is suprising that you say that frame_size cannot be set in dolby_e_init(), why does it matter? It can only be FRAME_SAMPLES, no other values can happen. In that sense it is similar to sample_fmt, which I also don't see why it is set in every decode call, and not only once, in init.
>
> Yes, this is not easy to see because the current code does not make this problem show up, but my initial target is a patch serie to support dolby_e in the wav container, and it makes it very clear.
> Please look at :
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20201019081929.1926-9-nicolas.gaullier@cji.paris/
> The issue is that the WAV container has typically a sample rate of 48000Hz which contains the s377m submux that embedd a 44800Hz stream ("pal"-DolbyE here) : all this is very typical, standard, DolbyE content, but there is a trick in compute_pkt_fields (if I remember correctly my testing) with early duration setting based on 48000Hz which results in a wrong value for the first frame.
> Maybe having a DolbyE parser would fix this (the 44800Hz would araise sooner), but currently the pts are broken.
>
> Here is the diff if I set frame_size at dolby_e_init:
> --- ./tests/ref/fate/s337m-wav  2020-12-15 18:02:28.166747900 +0100
> +++ tests/data/fate/s337m-wav   2021-01-05 10:27:01.193976500 +0100
> @@ -4,8 +4,8 @@
>  #sample_rate 0: 44800
>  #channel_layout 0: 63f
>  #channel_layout_name 0: 7.1
> -0,          0,          0,     1920,    11496, 0x05a9c147
> -0,       1920,       1920,     1920,    11496, 0x1d44d2b4
> -0,       3840,       3840,     1920,    11496, 0x4e078953
> -0,       5760,       5760,     1920,    11496, 0x1c73b1a1
> -0,       7680,       7680,     1920,    11262, 0xfa179fc8
> +0,          0,          0,     1792,    11496, 0x05a9c147
> +0,       1792,       1792,     1920,    11496, 0x1d44d2b4
> +0,       3712,       3712,     1920,    11496, 0x4e078953
> +0,       5632,       5632,     1920,    11496, 0x1c73b1a1
> +0,       7552,       7552,     1920,    11262, 0xfa179fc8
>
>>>> ---
>>>> libavcodec/dolby_e.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/libavcodec/dolby_e.c b/libavcodec/dolby_e.c index 
>>>> 429612ec08..b0e6d6aee3 100644
>>>> --- a/libavcodec/dolby_e.c
>>>> +++ b/libavcodec/dolby_e.c
>>>> @@ -577,6 +577,7 @@ static int filter_frame(DBEContext *s, AVFrame *frame)
>>>>  reorder = ch_reorder_n;
>>>>
>>>>  frame->nb_samples = FRAME_SAMPLES;
>>>> +    s->avctx->frame_size = FRAME_SAMPLES;
>>>>
> >If you still believe that setting this is required in every decode call, then I'd say it would be cleaner to set this at dolby_e_decode_frame where other avctx parameters are also set.
>
>>
>>
> >Thanks,
> >Marton
>
> I agree with you that sample_fmt and frame_size are both "const" and should probably be set at the same place wherever it is, and preferably at dolby_e_init.
> I cannot set frame_size in dolby_e_init because of the trick and sample_fmt is already set at dolby_e_decode_frame (for an unknown reason), maybe I should set frame_size in dolby_e_decode_frame too.
>
> I have just tested setting frame_size at dolby_e_decode_frame, and I confirm : yes, it works.
> This is not ideal but in the very short term, I really cannot see any other option : will you approve the patch if I set frame_size at dolby_e_decode_frame instead of filter_frame ? Should I amend my commit msg?
>

I still don't get it. It really does seem like a hack or a workaround to set the
frame size on every single frame.
In general, frame_size for decoders is read only. If something's touching it
apart from the decoder, then its an API misuse.
Nicolas Gaullier Jan. 8, 2021, 1:14 p.m. UTC | #5
>De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Lynne
>Envoyé : mardi 5 janvier 2021 16:26
>À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
>Objet : Re: [FFmpeg-devel] [PATCH] avcodec/dolby_e: set constant frame_size
>
>Jan 5, 2021, 10:43 by nicolas.gaullier@cji.paris:
>
>>>>> De : Nicolas Gaullier <nicolas.gaullier@cji.paris> Envoyé : mardi 
>>>>> 15 décembre 2020 18:13 À : ffmpeg-devel@ffmpeg.org Cc : Nicolas 
>>>>> Gaullier <nicolas.gaullier@cji.paris> Objet : [PATCH] 
>>>>> avcodec/dolby_e: set constant frame_size
>>>>>
>>>>> Fixes pts generation.
>>>>>
>>>>> Setting frame_size in dolby_e_init() or get_audio_frame_duration() can result in a bad duration value for the first packet if dolby_e is muxed in a container having a different sample_rate (ex:
>>>>> container @48KHz, DolbyE @44.8KHz).
>>>>> Maybe adding a parser to dolby_e would fix the issue and makes it possible to set frame_size at decoder init which seems the best place.
>>>>>
>> >I am not sure I understand this. It is suprising that you say that frame_size cannot be set in dolby_e_init(), why does it matter? It can only be FRAME_SAMPLES, no other values can happen. In that sense it is similar to sample_fmt, which I also don't see why it is set in every decode call, and not only once, in init.
>>
>> Yes, this is not easy to see because the current code does not make this problem show up, but my initial target is a patch serie to support dolby_e in the wav container, and it makes it very clear.
>> Please look at :
>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20201019081929.1926-
>> 9-nicolas.gaullier@cji.paris/ The issue is that the WAV container has 
>> typically a sample rate of 48000Hz which contains the s377m submux that embedd a 44800Hz stream ("pal"-DolbyE here) : all this is very typical, standard, DolbyE content, but there is a trick in compute_pkt_fields (if I remember correctly my testing) with early duration setting based on 48000Hz which results in a wrong value for the first frame.
>> Maybe having a DolbyE parser would fix this (the 44800Hz would araise sooner), but currently the pts are broken.
>>
>> Here is the diff if I set frame_size at dolby_e_init:
>> --- ./tests/ref/fate/s337m-wav  2020-12-15 18:02:28.166747900 +0100
>> +++ tests/data/fate/s337m-wav   2021-01-05 10:27:01.193976500 +0100
>> @@ -4,8 +4,8 @@
>>  #sample_rate 0: 44800
>>  #channel_layout 0: 63f
>>  #channel_layout_name 0: 7.1
>> -0,          0,          0,     1920,    11496, 0x05a9c147
>> -0,       1920,       1920,     1920,    11496, 0x1d44d2b4
>> -0,       3840,       3840,     1920,    11496, 0x4e078953
>> -0,       5760,       5760,     1920,    11496, 0x1c73b1a1
>> -0,       7680,       7680,     1920,    11262, 0xfa179fc8
>> +0,          0,          0,     1792,    11496, 0x05a9c147
>> +0,       1792,       1792,     1920,    11496, 0x1d44d2b4
>> +0,       3712,       3712,     1920,    11496, 0x4e078953
>> +0,       5632,       5632,     1920,    11496, 0x1c73b1a1
>> +0,       7552,       7552,     1920,    11262, 0xfa179fc8
>>
>>>>> ---
>>>>> libavcodec/dolby_e.c | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/libavcodec/dolby_e.c b/libavcodec/dolby_e.c index
>>>>> 429612ec08..b0e6d6aee3 100644
>>>>> --- a/libavcodec/dolby_e.c
>>>>> +++ b/libavcodec/dolby_e.c
>>>>> @@ -577,6 +577,7 @@ static int filter_frame(DBEContext *s, AVFrame 
>>>>> *frame)  reorder = ch_reorder_n;
>>>>>
>>>>>  frame->nb_samples = FRAME_SAMPLES;
>>>>> +    s->avctx->frame_size = FRAME_SAMPLES;
>>>>>
>> >If you still believe that setting this is required in every decode call, then I'd say it would be cleaner to set this at dolby_e_decode_frame where other avctx parameters are also set.
>>
>>>
>>>
>> >Thanks,
>> >Marton
>>
>> I agree with you that sample_fmt and frame_size are both "const" and should probably be set at the same place wherever it is, and preferably at dolby_e_init.
>> I cannot set frame_size in dolby_e_init because of the trick and sample_fmt is already set at dolby_e_decode_frame (for an unknown reason), maybe I should set frame_size in dolby_e_decode_frame too.
>>
>> I have just tested setting frame_size at dolby_e_decode_frame, and I confirm : yes, it works.
>> This is not ideal but in the very short term, I really cannot see any other option : will you approve the patch if I set frame_size at dolby_e_decode_frame instead of filter_frame ? Should I amend my commit msg?
>>
>
>I still don't get it. It really does seem like a hack or a workaround to set the frame size on every single frame.
>In general, frame_size for decoders is read only. If something's touching it apart from the decoder, then its an API misuse.

Thank you for your quick feedback!
Yes, you're right, it is somewhat a workaround and I suggested in the commit msg that adding a parser for dolby_e would fix the issue.
For me, the switch-case in get_audio_frame_duration with hardcoded frame_sizes also already sounds like workarounds and I thought that adding a parser to solely fix my case "wav support for dolbye" would be overkill.
Anyway, maybe there will be other benefits for having a dolby_e parser in the future.
So okay, I drop this patch and send a proposal for a dolby_e parser as soon as I get some time.
Nicolas
diff mbox series

Patch

diff --git a/libavcodec/dolby_e.c b/libavcodec/dolby_e.c
index 429612ec08..b0e6d6aee3 100644
--- a/libavcodec/dolby_e.c
+++ b/libavcodec/dolby_e.c
@@ -577,6 +577,7 @@  static int filter_frame(DBEContext *s, AVFrame *frame)
         reorder = ch_reorder_n;
 
     frame->nb_samples = FRAME_SAMPLES;
+    s->avctx->frame_size = FRAME_SAMPLES;
     if ((ret = ff_get_buffer(s->avctx, frame, 0)) < 0)
         return ret;