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

[Lynne]

Jan 5, 2021, 10:43 by nicolas.gaullier at cji.paris:

>>>> 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?
>

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.