>De : ffmpeg-devel <ffmpeg-devel-boun...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Reply via email to