On Tue, Apr 12, 2022 at 12:40 PM Paul B Mahol <one...@gmail.com> wrote:
> On Mon, Apr 11, 2022 at 10:59 PM Wang Cao < > wangcao-at-google....@ffmpeg.org> > wrote: > > > On Sat, Apr 9, 2022 at 5:35 AM Paul B Mahol <one...@gmail.com> wrote: > > > > > On Fri, Apr 8, 2022 at 10:41 PM Wang Cao < > > wangcao-at-google....@ffmpeg.org > > > > > > > wrote: > > > > > > > On Fri, Apr 8, 2022 at 11:40 AM Paul B Mahol <one...@gmail.com> > wrote: > > > > > > > > > On Thu, Apr 7, 2022 at 11:56 PM Wang Cao < > > > > wangcao-at-google....@ffmpeg.org > > > > > > > > > > > wrote: > > > > > > > > > > > On Thu, Apr 7, 2022 at 12:44 AM Paul B Mahol <one...@gmail.com> > > > wrote: > > > > > > > > > > > > > On Wed, Apr 6, 2022 at 1:49 PM Paul B Mahol <one...@gmail.com> > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Apr 5, 2022 at 8:57 PM Wang Cao < > > > > > > > wangcao-at-google....@ffmpeg.org> > > > > > > > > wrote: > > > > > > > > > > > > > > > >> On Mon, Apr 4, 2022 at 3:28 PM Marton Balint <c...@passwd.hu > > > > > > wrote: > > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > On Mon, 4 Apr 2022, Paul B Mahol wrote: > > > > > > > >> > > > > > > > > >> > > On Sun, Mar 27, 2022 at 11:41 PM Marton Balint < > > > c...@passwd.hu > > > > > > > > > > > > wrote: > > > > > > > >> > > > > > > > > > >> > >> > > > > > > > >> > >> > > > > > > > >> > >> On Sat, 26 Mar 2022, Wang Cao wrote: > > > > > > > >> > >> > > > > > > > >> > >>> The change in the commit will add some samples to the > > end > > > of > > > > > the > > > > > > > >> audio > > > > > > > >> > >>> stream. The intention is to add a "zero_delay" option > > > > > eventually > > > > > > > to > > > > > > > >> not > > > > > > > >> > >>> have the delay in the begining the output from > alimiter > > > due > > > > to > > > > > > > >> > >>> lookahead. > > > > > > > >> > >> > > > > > > > >> > >> I was very much suprised to see that the alimiter > filter > > > > > actually > > > > > > > >> delays > > > > > > > >> > >> the audio - as in extra samples are inserted in the > > > beginning > > > > > and > > > > > > > >> some > > > > > > > >> > >> samples are cut in the end. This trashes A-V sync, so > it > > > is a > > > > > bug > > > > > > > >> IMHO. > > > > > > > >> > >> > > > > > > > >> > >> So unless somebody has some valid usecase for the > legacy > > > way > > > > of > > > > > > > >> > operation > > > > > > > >> > >> I'd just simply change it to be "zero delay" without > any > > > > > > additional > > > > > > > >> user > > > > > > > >> > >> option, in a single patch. > > > > > > > >> > >> > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > This is done by this patch in very complicated way and > > also > > > it > > > > > > > really > > > > > > > >> > > should be optional. > > > > > > > >> > > > > > > > > >> > But why does it make sense to keep the current (IMHO > buggy) > > > > > > > operational > > > > > > > >> > mode which adds silence in the beginning and trims the > end? > > I > > > > > > > understand > > > > > > > >> > that the original implementation worked like this, but > > > > libavfilter > > > > > > has > > > > > > > >> > packet timestamps and N:M filtering so there is absolutely > > no > > > > > reason > > > > > > > to > > > > > > > >> > use an 1:1 implementation and live with its limitations. > > > > > > > >> > > > > > > > > >> Hello Paul and Marton, thank you so much for taking time to > > > review > > > > > my > > > > > > > >> patch. > > > > > > > >> I totally understand that my patch may seem a little bit > > > > complicated > > > > > > > but I > > > > > > > >> can > > > > > > > >> show with a FATE test that if we set the alimiter to behave > > as a > > > > > > > >> passthrough filter, > > > > > > > >> the output frames will be the same from "framecrc" with my > > > patch. > > > > > The > > > > > > > >> existing > > > > > > > >> behavior will not work for all gapless audio processing. > > > > > > > >> > > > > > > > >> The complete patch to fix this issue is at > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220330210314.2055201-1-wang...@google.com/ > > > > > > > >> > > > > > > > >> Regarding Paul's concern, I personally don't have any > > preference > > > > > > whether > > > > > > > >> to > > > > > > > >> put > > > > > > > >> the patch as an extra option or not. With respect to the > > > > > > implementation, > > > > > > > >> the patch > > > > > > > >> is the best I can think of by preserving as much information > > as > > > > > > possible > > > > > > > >> from input > > > > > > > >> frames. I also understand it may break concept that > > > "filter_frame" > > > > > > > outputs > > > > > > > >> one frame > > > > > > > >> at a time. For alimiter with my patch, depending on the size > > of > > > > the > > > > > > > >> lookahead buffer, > > > > > > > >> it may take a few frames before one output frame can be > > > generated. > > > > > > This > > > > > > > is > > > > > > > >> inevitable > > > > > > > >> to compensate for the delay of the lookahead buffer. > > > > > > > >> > > > > > > > >> Thanks again for reviewing my patch and I'm looking forward > to > > > > > hearing > > > > > > > >> from > > > > > > > >> you :) > > > > > > > >> > > > > > > > > > > > > > > > > Better than (because its no more 1 frame X nb_samples in, 1 > > > frame X > > > > > > > > nb_samples out) to replace .filter_frame/.request_frame with > > > > > .activate > > > > > > > > logic. > > > > > > > > > > > > > > > > And make this output delay compensation filtering optional. > > > > > > > > > > > > > > > > In this process make sure that output PTS frame timestamps > are > > > > > > unchanged > > > > > > > > from input one, by keeping reference of needed frames in > filter > > > > > queue. > > > > > > > > > > > > > > > > Look how speechnorm/dynaudnorm does it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Alternatively, use current logic in ladspa filter, (also add > > option > > > > > with > > > > > > > same name). > > > > > > > > > > > > > > This would need less code, and probably better approach, as > there > > > is > > > > no > > > > > > > extra latency introduced. > > > > > > > > > > > > > > Than mapping 1:1 between same number of samples per frame is > not > > > hold > > > > > any > > > > > > > more, but I think that is not much important any more. > > > > > > > > > > > > > Thank you for replying to me with your valuable feedback! I have > > > > checked > > > > > > af_ladspa > > > > > > and the "request_frame" function in af_ladspa looks similar to > what > > > I'm > > > > > > doing. The > > > > > > difference comes from the fact that I need an internal frame > buffer > > > to > > > > > keep > > > > > > track of > > > > > > output frames. Essentially I add a frame to the internal buffer > > when > > > an > > > > > > input frame > > > > > > comes in. The frames in this buffer will be the future output > > frames. > > > > We > > > > > > start writing > > > > > > these output frame data buffers only when the internal lookahead > > > buffer > > > > > has > > > > > > been filled. > > > > > > When there is no more input frames, we flushing all the remaining > > > data > > > > in > > > > > > the internal > > > > > > frame buffers and lookahead buffers. Can you advise on my > approach > > > > here? > > > > > > Thank you! > > > > > > > > > > > > I can put my current implementation of "filter_frame" and > > > > "request_frame" > > > > > > into the "activate" approach as you suggested with > > > > speechnorm/dynaudnorm. > > > > > > > > > > > > > > > > No need to wait for all buffers to fill up, only lookahead buffer. > > > > > > > > > > Just trim initial samples that is size of lookahead, and than start > > > > > outputing samples > > > > > just once you get whatever modulo of current frame number of > samples. > > > > > > > > > > So there should not be need for extra buffers to keep audio > samples. > > > > > Just buffers to keep input pts and number of samples of previous > > input > > > > > frames, like in ladspa filter. > > > > > > > > > Thank you for the suggestion! From my understanding, we have two ways > > to > > > > achieve > > > > "zero_delay" functionality here. > > > > > > > > Option 1: as you mentioned, we can trim the initial samples of > > lookahead > > > > size. > > > > The size of the lookahead buffer can be multiple frames. For example > > when > > > > the > > > > attack is 0.08 second, sample rate is 44100 and frame size is 1024, > the > > > > lookahead > > > > buffer size is about 3 frames so the filter needs to see at least 3 > > input > > > > audio frames > > > > before it can output one audio frame. We also need to make > assumptions > > > > about the > > > > size of the audio frame (meaning the number of audio samples per > frame) > > > > when flushing. > > > > The frame is probably 1024 conventionally but I think it's better to > > make > > > > less assumption > > > > as possible to allow the filter to be used as flexible as possible. > > > > > > > > Option 2: this is what I proposed before. We basically map the same > > > number > > > > of input > > > > frames to the output and we also make sure everything about the frame > > the > > > > same as > > > > the input except for the audio signal data itself, which will be > > changed > > > by > > > > whatever > > > > processing alimiter has to do with that. I think it is safer to make > > the > > > > filter only work on > > > > the signal itself. It can help other people who use this filter > without > > > > worrying about > > > > any unexpected change on the frame. The downside is that the filter > > > > internally needs to > > > > store some empty frames, which will be written as the lookahead > buffer > > is > > > > filled. > > > > > > > > I don't see any performance difference between these two options but > > > option > > > > 2 looks > > > > better to me because it works solely on the signals without any > changes > > > on > > > > the frame > > > > > > > > > > option 1 does not add extra delay in processing chain at all, and > option > > 2 > > > adds extra delay. > > > > > > Just look at latest version of af_ladspa.c filter code. > > > > > Hi Paul, I have checked af_ladspa filter. Option 1 certainly doesn't > > introduce any delay. > > Option 2 will at most introduce one audio frame of delay in the > processing > > chain. Option > > 2 needs to fill the lookahead buffer and some frames of samples to push > the > > data in the > > lookahead buffer out for one output audio frame. The processing delay is > > probably not a > > big concern. > > > > The reason I proposed option 2 is about the sync of metadata for the > output > > frames: > > > > It appears to me that the only metadata we need to worry about is PTS and > > the number of > > samples in af_ladspa. However, AVFrame has other fields that also seem to > > specify some metadata > > for the frame: > > 1. AVDictionary *metadata > > 2. void *opaque (this seems to be client-specific) > > 3. AVFrameSideData** side_data (this is not handled by > > av_frame_copy_props). > > If we go for option 1, it is likely these fields in the input frame will > > not be mapped to the corresponding > > output frames. I believe this is also an unexpected behavior for other > > users who rely on these fields. I > > understand other filters may not consider this as important but > > conceptually I believe it is better to make > > the filter focus on changing what it is supposed to, which is the audio > > signal itself. > > > > Looking forward to hearing your opinion on this, thanks! > > > > > AVFrame Metadata is mostly useful for cases when input frame after > filtering > with filter that adds metadata no longer changes output frames later in > filter graph. > Thus using something like astats=metadata=1,alimiter is not very useful and > logical. > > Why by, lightly insisting on no extra delay/latency introduction, and > prefer 1, option for alimiter filter > is mostly because in audio domain, latency is relevant and important > subject for most audio processing specially for limiters. > So it is very important to keep it at minimum if possible. > Thank you so much for introducing me with the context. As I understand the possible metadata can be, I think I have fully understood what I can do. Thanks! -- Wang Cao | Software Engineer | wang...@google.com | 650-203-7807 _______________________________________________ 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".
