On 4/9/2020 1:45 PM, Philip Langdale wrote: > On Thu, 9 Apr 2020 11:20:09 -0300 > James Almer <jamr...@gmail.com> wrote: > >> On 4/8/2020 7:04 PM, Philip Langdale wrote: >>> On Wed, 8 Apr 2020 14:58:36 -0300 >>> James Almer <jamr...@gmail.com> wrote: >>> >>>> Signed-off-by: James Almer <jamr...@gmail.com> >>>> --- >>>> This removes the encode2() implementation as it'll never be used >>>> if a receive_packet() one exists, and the flush() implementation >>>> since according to Anton Khirnov avcodec_flush_buffers() is not >>>> meant to be used with encoders, where its behavior is undefined. >>> >>> Nevertheless, there is a use case for flushing an encoder (see >>> 3ea705), and when you call avcodec_flush_buffers() in nvenc, it >>> does the right thing. >>> >>> Removing this will return us to the world before that change where >>> there was no way to flush the encoder, and the original bug reporter >>> was asking about adding an API to do it. >>> >>> It seems the right thing to do is to define the behaviour - which >>> seems reasonable as-is today and documment that encoders can >>> implement flush if necessary. Or we'll just end up adding a new API >>> that flushes encoders but has a different name... >>> >>> --phil >> >> I'm not against it, but as Marton said the function as it is is not >> enough. The changes Anton asked me to remove would need to be re-added >> (trivial to do), and the threading related code would probably need >> some changes, if anything to not call decoding specific functions on >> an encoder context. >> >> I like the codec capabilities suggestion from Marton. Could you look >> into it? I'll change this patch to not remove the flush call while at >> it. > > Yes, please leave the flush as-is for now so we don't regress. At the > very least I think adding an encoder vs decoder path into the function > makes sense and we can skip all the decoder specific parts. Today, the > emergent behaviour is that an encoder is only affected by the flush > call if it provides a flush callback - none of the other parts will > alter its state
No, right now buffer_pkt is unrefferenced, and buffer_pkt_valid and draining are reset to 0 by this function. All of them fields used by the encode API, before and after this patchset. If an encoder doesn't have a flush() callback, then the encoder state will be altered and things could behave in an unpredictable way. , so perhaps the simple thing to do is have the encoder > path just call the callback if it's there and naturally do nothing if > not. That removes the need for a separate capability flag. Yes, but how will the user know what encoder lets them flush without the need to recreate the context, and which doesn't? avcodec_flush_buffers() could do something or it could not, and the user will never know because it doesn't return any kind of error code or result. avcodec_flush_buffers() can easily be turned into a no-op for encoders that don't implement a flush callback, but a codec cap or a similar solution would let user know when he has no other option than to recreate the context. _______________________________________________ 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".
