[FFmpeg-devel] [PATCH 06/11] avformat: add av_abort_output() function

[Jan Sebechlebsky]

On 08/03/2016 03:15 PM, Nicolas George wrote:
> Le sextidi 16 thermidor, an CCXXIV, sebechlebskyjan at gmail.com a écrit :
>> From: Jan Sebechlebsky <sebechlebskyjan at gmail.com>
>>
>> Signed-off-by: Jan Sebechlebsky <sebechlebskyjan at gmail.com>
>> ---
>>   libavformat/avformat.h | 14 ++++++++++++++
>>   libavformat/mux.c      | 16 ++++++++++++++++
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 9191c69..9173908 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -2510,6 +2510,8 @@ int av_write_uncoded_frame_query(AVFormatContext *s, int stream_index);
>>    *
>>    * If AVFMT_FLAG_NONBLOCK is set, this call may return AVERROR(EAGAIN)
>>    * meaning the operation is pending and the call should be repeated.
>> + * If caller decides to abort operation (after too many calls have returned
>> + * AVERROR(EAGAIN)), it can be done by calling @ref av_abort_output().
>>    *
>>    * @param s media file handle
>>    * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
>> @@ -2518,6 +2520,18 @@ int av_write_uncoded_frame_query(AVFormatContext *s, int stream_index);
>>   int av_write_trailer(AVFormatContext *s);
>>   
>>   /**
>> + * Abort non-blocking muxer operation and free private data.
>> + *
>> + * May only be called after a successful call to avformat_write_header,
>> + * and used only with muxer operating in non-blocking mode (AVFMT_FLAG_NONBLOCK)
>> + * must be set.
>> + *
>> + * @param s media file handle
>> + * return >= 0 on success, negative AVERROR on error
>> + */
>> +int av_abort_output(AVFormatContext *s);
> The other functions are called av_write_something() or
> avformat_write_something(): maybe avformat_write_abort()?
I'll change to avformat_write_abort.
> Also, it could call avformat_free_context() and set s to NULL, unless there
> is some use in reusing the context?
I thought of this function as about an alternative to av_write_trailer - 
so you can replace one call for another and get the same behaviour (from 
the "outside" point of view). But it's not big deal, if you wish I can 
change this.
>> +
>> +/**
>>    * Return the output format in the list of registered output formats
>>    * which best matches the provided parameters, or return NULL if
>>    * there is no match.
>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>> index bc9c98f..888a9f1 100644
>> --- a/libavformat/mux.c
>> +++ b/libavformat/mux.c
>> @@ -1267,6 +1267,22 @@ fail:
>>       return ret;
>>   }
>>   
>> +int av_abort_output(AVFormatContext *s)
>> +{
>> +    int ret;
>> +
>> +    if (!(s->flags & AVFMT_FLAG_NONBLOCK))
>> +        return AVERROR(EINVAL);
> Since the application should be able to know whether the muxer is in
> blocking mode, it could be an assert.
>
> But is it really necessary? Applications in blocking mode may want to exit
> immediately too.
I think there is no reason for blocking muxer to call this, it shouldn't 
avoid calling av_write_trailer to free resources. But I can leave here 
the av_write_trailer call below and remove the check above, in that case 
this would became just av_write_trailer wrapper for blocking muxer.
>
> Anyway, any of these changes would allow to make the function void.
>
>> +
>> +    ret = av_write_trailer(s);
>> +    if (ret == AVERROR(EAGAIN)) {
> Is it useful? The application could try and write the trailer itself,
> possibly allowing a little more time to finish. And only call
> av_abort_output() when it really wants to stop now.
I thought it might be good idea to call write_trailer before actually 
calling deinit. User can still do as many av_write_trailer attempts as 
he wants before, but if he does none (for example decides to abort 
output not because of write_trailer timeout, but because of some other 
error or signal caught before all packets are send to muxer) this will 
at first attempt to finish in a graceful way.
If you're ok with that I would keep that here and I'll remove the 
AVFMT_FLAG_NONBLOCK flag check since then it's safe to call this instead 
of av_write_trailer regardless of whether muxer is operating in 
blocking/nonblocking mode.
>> +        deinit_muxer(s);
> There is a subtle pitfall here: if the muxer does not have a deinit function
> and av_write_trailer() returned EAGAIN (or was not called like I suggest),
> then the resources leak.
>
> Fortunately, we currently do not have any muxer that both supports
> non-blocking mode and has a potentially blocking write_trailer(). We can
> decide that any future muxer that would must have a deinit() function. In
> that case, we can get away with the following steps:
>
>    1. If the muxer has a deinit() method, just call deinit_muxer().
>
>    2. Else, sabotage the context's AVIO stream so that it returns an error
>       for all operations and call av_write_trailer().
>
> The "sabotage" part of 2 is not very robust, but I think it is fine enough
> for our use case. In fact, I would not object if that detail was left as a
> TODO comment.
Maybe a good solution would be to document that nonblocking muxer must 
have deinit function and assert that during initialization in 
avformat_write_header?
     I think sabotaging AVIO is risky, since this will especially happen 
when the IO operation is pending. If the interrupt callback (or pointer) 
would be stored in AVIO context it could be replaced atomically to cause 
AVERROR_EXIT, but it is passed as argument to IO operations.

If you agree, I will do these changes:
  - rename the function to avformat_write_abort
  - remove the check for AVFMT_FLAG_NONBLOCK
  - send another patch documenting restriction for nonblocking muxer 
that it must have deinit function and all resources must be freed only 
there and adding assert to av_write_header.

Thanks for review!

Jan