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

[Nicolas Gaullier]

>De : ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> De la part de Lynne
>Envoyé : mardi 5 janvier 2021 16:26
>À : FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
>Objet : Re: [FFmpeg-devel] [PATCH] avcodec/dolby_e: set constant frame_size
>
>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 at 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