>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".
