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

[Nicolas Gaullier]

>>> De : Nicolas Gaullier <nicolas.gaullier at cji.paris> Envoyé : mardi 15 
>>> décembre 2020 18:13 À : ffmpeg-devel at ffmpeg.org Cc : Nicolas Gaullier 
>>> <nicolas.gaullier at 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