[FFmpeg-devel,v2,04/11] avformat/muxers: Add non-blocking mode support for av_write_trailer

Submitted by sebechlebskyjan@gmail.com on Aug. 11, 2016, 12:38 p.m.

Details

Message ID 1470919109-3327-1-git-send-email-sebechlebskyjan@gmail.com
State New
Headers show

Commit Message

sebechlebskyjan@gmail.com Aug. 11, 2016, 12:38 p.m.
From: Jan Sebechlebsky <sebechlebskyjan@gmail.com>

This makes av_write_trailer not to free the resources if write_trailer
call returns AVERROR(EAGAIN) allowing repeated calls of write_trailer of
non-blocking muxer.

Signed-off-by: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
---
 Changes since the last version of the patch:
 - Added assert to the part of the code dealing with flushing
   interleaved packets which should not be entered if 
   muxer in non-blocking mode is used.
   (also there is assert for the same condition added into
    av_interleaved_write_packet in one of the following 
    patches).

 libavformat/avformat.h |  6 +++++-
 libavformat/mux.c      | 10 ++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

Comments

Michael Niedermayer Aug. 22, 2016, 7:51 a.m.
On Thu, Aug 11, 2016 at 02:38:29PM +0200, sebechlebskyjan@gmail.com wrote:
> From: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
> 
> This makes av_write_trailer not to free the resources if write_trailer
> call returns AVERROR(EAGAIN) allowing repeated calls of write_trailer of
> non-blocking muxer.
> 
> Signed-off-by: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
> ---
>  Changes since the last version of the patch:
>  - Added assert to the part of the code dealing with flushing
>    interleaved packets which should not be entered if 
>    muxer in non-blocking mode is used.
>    (also there is assert for the same condition added into
>     av_interleaved_write_packet in one of the following 
>     patches).
> 
>  libavformat/avformat.h |  6 +++++-
>  libavformat/mux.c      | 10 ++++++++--
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index d8a6cf3..2cc3156 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -2510,8 +2510,12 @@ int av_write_uncoded_frame_query(AVFormatContext *s, int stream_index);
>   *
>   * May only be called after a successful call to avformat_write_header.
>   *
> + * If AVFMT_FLAG_NONBLOCK is set, this call may return AVERROR(EAGAIN)
> + * meaning the operation is pending and the call should be repeated.
> + *
>   * @param s media file handle
> - * @return 0 if OK, AVERROR_xxx on error
> + * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
> + *         other AVERROR on error
>   */
>  int av_write_trailer(AVFormatContext *s);
>  
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index e9973ed..3ae924c 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -1204,11 +1204,14 @@ int av_write_trailer(AVFormatContext *s)
>      for (;; ) {
>          AVPacket pkt;
>          ret = interleave_packet(s, &pkt, NULL, 1);
> -        if (ret < 0)
> -            goto fail;
>          if (!ret)
>              break;
>  
> +        av_assert0(!(s->flags & AVFMT_FLAG_NONBLOCK));

this would abort on any error not just EAGAIN

[...]
sebechlebskyjan@gmail.com Aug. 22, 2016, 2:27 p.m.
On 08/22/2016 09:51 AM, Michael Niedermayer wrote:
> On Thu, Aug 11, 2016 at 02:38:29PM +0200, sebechlebskyjan@gmail.com wrote:
>> From: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
>>
>> This makes av_write_trailer not to free the resources if write_trailer
>> call returns AVERROR(EAGAIN) allowing repeated calls of write_trailer of
>> non-blocking muxer.
>>
>> Signed-off-by: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
>> ---
>>   Changes since the last version of the patch:
>>   - Added assert to the part of the code dealing with flushing
>>     interleaved packets which should not be entered if
>>     muxer in non-blocking mode is used.
>>     (also there is assert for the same condition added into
>>      av_interleaved_write_packet in one of the following
>>      patches).
>>
>>   libavformat/avformat.h |  6 +++++-
>>   libavformat/mux.c      | 10 ++++++++--
>>   2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index d8a6cf3..2cc3156 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -2510,8 +2510,12 @@ int av_write_uncoded_frame_query(AVFormatContext *s, int stream_index);
>>    *
>>    * May only be called after a successful call to avformat_write_header.
>>    *
>> + * If AVFMT_FLAG_NONBLOCK is set, this call may return AVERROR(EAGAIN)
>> + * meaning the operation is pending and the call should be repeated.
>> + *
>>    * @param s media file handle
>> - * @return 0 if OK, AVERROR_xxx on error
>> + * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
>> + *         other AVERROR on error
>>    */
>>   int av_write_trailer(AVFormatContext *s);
>>   
>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>> index e9973ed..3ae924c 100644
>> --- a/libavformat/mux.c
>> +++ b/libavformat/mux.c
>> @@ -1204,11 +1204,14 @@ int av_write_trailer(AVFormatContext *s)
>>       for (;; ) {
>>           AVPacket pkt;
>>           ret = interleave_packet(s, &pkt, NULL, 1);
>> -        if (ret < 0)
>> -            goto fail;
>>           if (!ret)
>>               break;
>>   
>> +        av_assert0(!(s->flags & AVFMT_FLAG_NONBLOCK));
> this would abort on any error not just EAGAIN
I think it will abort in case interleave_packets does not return 0 from 
the first call in loop, which means that interleaving was used (because 
there are some packets to be flushed) and that situation cannot happen 
with AVFMT_FLAG_NONBLOCK set when interleaving is forbidded. The next 
patch also adds assert to av_interleaved_write_packet. But I think the 
assert here is on the right place, or have I misunderstood the problem 
you're pointing out?

Regards,
Jan
Michael Niedermayer Aug. 22, 2016, 2:49 p.m.
On Mon, Aug 22, 2016 at 04:27:16PM +0200, Jan Sebechlebsky wrote:
> 
> 
> On 08/22/2016 09:51 AM, Michael Niedermayer wrote:
> >On Thu, Aug 11, 2016 at 02:38:29PM +0200, sebechlebskyjan@gmail.com wrote:
> >>From: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
> >>
> >>This makes av_write_trailer not to free the resources if write_trailer
> >>call returns AVERROR(EAGAIN) allowing repeated calls of write_trailer of
> >>non-blocking muxer.
> >>
> >>Signed-off-by: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
> >>---
> >>  Changes since the last version of the patch:
> >>  - Added assert to the part of the code dealing with flushing
> >>    interleaved packets which should not be entered if
> >>    muxer in non-blocking mode is used.
> >>    (also there is assert for the same condition added into
> >>     av_interleaved_write_packet in one of the following
> >>     patches).
> >>
> >>  libavformat/avformat.h |  6 +++++-
> >>  libavformat/mux.c      | 10 ++++++++--
> >>  2 files changed, 13 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> >>index d8a6cf3..2cc3156 100644
> >>--- a/libavformat/avformat.h
> >>+++ b/libavformat/avformat.h
> >>@@ -2510,8 +2510,12 @@ int av_write_uncoded_frame_query(AVFormatContext *s, int stream_index);
> >>   *
> >>   * May only be called after a successful call to avformat_write_header.
> >>   *
> >>+ * If AVFMT_FLAG_NONBLOCK is set, this call may return AVERROR(EAGAIN)
> >>+ * meaning the operation is pending and the call should be repeated.
> >>+ *
> >>   * @param s media file handle
> >>- * @return 0 if OK, AVERROR_xxx on error
> >>+ * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
> >>+ *         other AVERROR on error
> >>   */
> >>  int av_write_trailer(AVFormatContext *s);
> >>diff --git a/libavformat/mux.c b/libavformat/mux.c
> >>index e9973ed..3ae924c 100644
> >>--- a/libavformat/mux.c
> >>+++ b/libavformat/mux.c
> >>@@ -1204,11 +1204,14 @@ int av_write_trailer(AVFormatContext *s)
> >>      for (;; ) {
> >>          AVPacket pkt;
> >>          ret = interleave_packet(s, &pkt, NULL, 1);
> >>-        if (ret < 0)
> >>-            goto fail;
> >>          if (!ret)
> >>              break;
> >>+        av_assert0(!(s->flags & AVFMT_FLAG_NONBLOCK));
> >this would abort on any error not just EAGAIN
> I think it will abort in case interleave_packets does not return 0
> from the first call in loop, which means that interleaving was used
> (because there are some packets to be flushed) and that situation
> cannot happen with AVFMT_FLAG_NONBLOCK set when interleaving is
> forbidded. The next patch also adds assert to
> av_interleaved_write_packet. But I think the assert here is on the
> right place, or have I misunderstood the problem you're pointing
> out?

I thought interleave_packet can return AVERROR(ENOMEM)
maybe this is not possible, still it seems non-robust to assert if
it errors out

[...]
sebechlebskyjan@gmail.com Aug. 25, 2016, 10:59 p.m.
On 08/22/2016 04:49 PM, Michael Niedermayer wrote:

> On Mon, Aug 22, 2016 at 04:27:16PM +0200, Jan Sebechlebsky wrote:
>>
>> On 08/22/2016 09:51 AM, Michael Niedermayer wrote:
>>> On Thu, Aug 11, 2016 at 02:38:29PM +0200, sebechlebskyjan@gmail.com wrote:
>>>> From: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
>>>>
>>>> This makes av_write_trailer not to free the resources if write_trailer
>>>> call returns AVERROR(EAGAIN) allowing repeated calls of write_trailer of
>>>> non-blocking muxer.
>>>>
>>>> Signed-off-by: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
>>>> ---
>>>>   Changes since the last version of the patch:
>>>>   - Added assert to the part of the code dealing with flushing
>>>>     interleaved packets which should not be entered if
>>>>     muxer in non-blocking mode is used.
>>>>     (also there is assert for the same condition added into
>>>>      av_interleaved_write_packet in one of the following
>>>>      patches).
>>>>
>>>>   libavformat/avformat.h |  6 +++++-
>>>>   libavformat/mux.c      | 10 ++++++++--
>>>>   2 files changed, 13 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>>> index d8a6cf3..2cc3156 100644
>>>> --- a/libavformat/avformat.h
>>>> +++ b/libavformat/avformat.h
>>>> @@ -2510,8 +2510,12 @@ int av_write_uncoded_frame_query(AVFormatContext *s, int stream_index);
>>>>    *
>>>>    * May only be called after a successful call to avformat_write_header.
>>>>    *
>>>> + * If AVFMT_FLAG_NONBLOCK is set, this call may return AVERROR(EAGAIN)
>>>> + * meaning the operation is pending and the call should be repeated.
>>>> + *
>>>>    * @param s media file handle
>>>> - * @return 0 if OK, AVERROR_xxx on error
>>>> + * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
>>>> + *         other AVERROR on error
>>>>    */
>>>>   int av_write_trailer(AVFormatContext *s);
>>>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>>>> index e9973ed..3ae924c 100644
>>>> --- a/libavformat/mux.c
>>>> +++ b/libavformat/mux.c
>>>> @@ -1204,11 +1204,14 @@ int av_write_trailer(AVFormatContext *s)
>>>>       for (;; ) {
>>>>           AVPacket pkt;
>>>>           ret = interleave_packet(s, &pkt, NULL, 1);
>>>> -        if (ret < 0)
>>>> -            goto fail;
>>>>           if (!ret)
>>>>               break;
>>>> +        av_assert0(!(s->flags & AVFMT_FLAG_NONBLOCK));
>>> this would abort on any error not just EAGAIN
>> I think it will abort in case interleave_packets does not return 0
>> from the first call in loop, which means that interleaving was used
>> (because there are some packets to be flushed) and that situation
>> cannot happen with AVFMT_FLAG_NONBLOCK set when interleaving is
>> forbidded. The next patch also adds assert to
>> av_interleaved_write_packet. But I think the assert here is on the
>> right place, or have I misunderstood the problem you're pointing
>> out?
> I thought interleave_packet can return AVERROR(ENOMEM)
> maybe this is not possible, still it seems non-robust to assert if
> it errors out
>
I think it cannot return AVERROR(ENOMEM) in case interleaving was not 
used. But maybe I can remove assert from this function since there will 
be assert in av_interleaved_write_frame?

Regards,
Jan
sebechlebskyjan@gmail.com Sept. 1, 2016, 7:45 p.m.
On 08/26/2016 12:59 AM, Jan Sebechlebsky wrote:
> On 08/22/2016 04:49 PM, Michael Niedermayer wrote:
>
>> On Mon, Aug 22, 2016 at 04:27:16PM +0200, Jan Sebechlebsky wrote:
>>>
>>> On 08/22/2016 09:51 AM, Michael Niedermayer wrote:
>>>> On Thu, Aug 11, 2016 at 02:38:29PM +0200, sebechlebskyjan@gmail.com 
>>>> wrote:
>>>>> From: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
>>>>>
>>>>> This makes av_write_trailer not to free the resources if 
>>>>> write_trailer
>>>>> call returns AVERROR(EAGAIN) allowing repeated calls of 
>>>>> write_trailer of
>>>>> non-blocking muxer.
>>>>>
>>>>> Signed-off-by: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
>>>>> ---
>>>>>   Changes since the last version of the patch:
>>>>>   - Added assert to the part of the code dealing with flushing
>>>>>     interleaved packets which should not be entered if
>>>>>     muxer in non-blocking mode is used.
>>>>>     (also there is assert for the same condition added into
>>>>>      av_interleaved_write_packet in one of the following
>>>>>      patches).
>>>>>
>>>>>   libavformat/avformat.h |  6 +++++-
>>>>>   libavformat/mux.c      | 10 ++++++++--
>>>>>   2 files changed, 13 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>>>> index d8a6cf3..2cc3156 100644
>>>>> --- a/libavformat/avformat.h
>>>>> +++ b/libavformat/avformat.h
>>>>> @@ -2510,8 +2510,12 @@ int 
>>>>> av_write_uncoded_frame_query(AVFormatContext *s, int stream_index);
>>>>>    *
>>>>>    * May only be called after a successful call to 
>>>>> avformat_write_header.
>>>>>    *
>>>>> + * If AVFMT_FLAG_NONBLOCK is set, this call may return 
>>>>> AVERROR(EAGAIN)
>>>>> + * meaning the operation is pending and the call should be repeated.
>>>>> + *
>>>>>    * @param s media file handle
>>>>> - * @return 0 if OK, AVERROR_xxx on error
>>>>> + * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
>>>>> + *         other AVERROR on error
>>>>>    */
>>>>>   int av_write_trailer(AVFormatContext *s);
>>>>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>>>>> index e9973ed..3ae924c 100644
>>>>> --- a/libavformat/mux.c
>>>>> +++ b/libavformat/mux.c
>>>>> @@ -1204,11 +1204,14 @@ int av_write_trailer(AVFormatContext *s)
>>>>>       for (;; ) {
>>>>>           AVPacket pkt;
>>>>>           ret = interleave_packet(s, &pkt, NULL, 1);
>>>>> -        if (ret < 0)
>>>>> -            goto fail;
>>>>>           if (!ret)
>>>>>               break;
>>>>> +        av_assert0(!(s->flags & AVFMT_FLAG_NONBLOCK));
>>>> this would abort on any error not just EAGAIN
>>> I think it will abort in case interleave_packets does not return 0
>>> from the first call in loop, which means that interleaving was used
>>> (because there are some packets to be flushed) and that situation
>>> cannot happen with AVFMT_FLAG_NONBLOCK set when interleaving is
>>> forbidded. The next patch also adds assert to
>>> av_interleaved_write_packet. But I think the assert here is on the
>>> right place, or have I misunderstood the problem you're pointing
>>> out?
>> I thought interleave_packet can return AVERROR(ENOMEM)
>> maybe this is not possible, still it seems non-robust to assert if
>> it errors out
>>
> I think it cannot return AVERROR(ENOMEM) in case interleaving was not 
> used. But maybe I can remove assert from this function since there 
> will be assert in av_interleaved_write_frame?
>
> Regards,
> Jan
Should I remove the assert from the patch (since it will be in 
av_interleaved_write_frame)?

Regards,
Jan
Michael Niedermayer Sept. 1, 2016, 10:19 p.m.
On Thu, Sep 01, 2016 at 09:45:49PM +0200, Jan Sebechlebsky wrote:
> 
> 
> On 08/26/2016 12:59 AM, Jan Sebechlebsky wrote:
> >On 08/22/2016 04:49 PM, Michael Niedermayer wrote:
> >
> >>On Mon, Aug 22, 2016 at 04:27:16PM +0200, Jan Sebechlebsky wrote:
> >>>
> >>>On 08/22/2016 09:51 AM, Michael Niedermayer wrote:
> >>>>On Thu, Aug 11, 2016 at 02:38:29PM +0200,
> >>>>sebechlebskyjan@gmail.com wrote:
> >>>>>From: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
> >>>>>
> >>>>>This makes av_write_trailer not to free the resources if
> >>>>>write_trailer
> >>>>>call returns AVERROR(EAGAIN) allowing repeated calls of
> >>>>>write_trailer of
> >>>>>non-blocking muxer.
> >>>>>
> >>>>>Signed-off-by: Jan Sebechlebsky <sebechlebskyjan@gmail.com>
> >>>>>---
> >>>>>  Changes since the last version of the patch:
> >>>>>  - Added assert to the part of the code dealing with flushing
> >>>>>    interleaved packets which should not be entered if
> >>>>>    muxer in non-blocking mode is used.
> >>>>>    (also there is assert for the same condition added into
> >>>>>     av_interleaved_write_packet in one of the following
> >>>>>     patches).
> >>>>>
> >>>>>  libavformat/avformat.h |  6 +++++-
> >>>>>  libavformat/mux.c      | 10 ++++++++--
> >>>>>  2 files changed, 13 insertions(+), 3 deletions(-)
> >>>>>
> >>>>>diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> >>>>>index d8a6cf3..2cc3156 100644
> >>>>>--- a/libavformat/avformat.h
> >>>>>+++ b/libavformat/avformat.h
> >>>>>@@ -2510,8 +2510,12 @@ int
> >>>>>av_write_uncoded_frame_query(AVFormatContext *s, int
> >>>>>stream_index);
> >>>>>   *
> >>>>>   * May only be called after a successful call to
> >>>>>avformat_write_header.
> >>>>>   *
> >>>>>+ * If AVFMT_FLAG_NONBLOCK is set, this call may return
> >>>>>AVERROR(EAGAIN)
> >>>>>+ * meaning the operation is pending and the call should be repeated.
> >>>>>+ *
> >>>>>   * @param s media file handle
> >>>>>- * @return 0 if OK, AVERROR_xxx on error
> >>>>>+ * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
> >>>>>+ *         other AVERROR on error
> >>>>>   */
> >>>>>  int av_write_trailer(AVFormatContext *s);
> >>>>>diff --git a/libavformat/mux.c b/libavformat/mux.c
> >>>>>index e9973ed..3ae924c 100644
> >>>>>--- a/libavformat/mux.c
> >>>>>+++ b/libavformat/mux.c
> >>>>>@@ -1204,11 +1204,14 @@ int av_write_trailer(AVFormatContext *s)
> >>>>>      for (;; ) {
> >>>>>          AVPacket pkt;
> >>>>>          ret = interleave_packet(s, &pkt, NULL, 1);
> >>>>>-        if (ret < 0)
> >>>>>-            goto fail;
> >>>>>          if (!ret)
> >>>>>              break;
> >>>>>+        av_assert0(!(s->flags & AVFMT_FLAG_NONBLOCK));
> >>>>this would abort on any error not just EAGAIN
> >>>I think it will abort in case interleave_packets does not return 0
> >>>from the first call in loop, which means that interleaving was used
> >>>(because there are some packets to be flushed) and that situation
> >>>cannot happen with AVFMT_FLAG_NONBLOCK set when interleaving is
> >>>forbidded. The next patch also adds assert to
> >>>av_interleaved_write_packet. But I think the assert here is on the
> >>>right place, or have I misunderstood the problem you're pointing
> >>>out?
> >>I thought interleave_packet can return AVERROR(ENOMEM)
> >>maybe this is not possible, still it seems non-robust to assert if
> >>it errors out
> >>
> >I think it cannot return AVERROR(ENOMEM) in case interleaving was
> >not used. But maybe I can remove assert from this function since
> >there will be assert in av_interleaved_write_frame?
> >
> >Regards,
> >Jan
> Should I remove the assert from the patch (since it will be in
> av_interleaved_write_frame)?

feel free to apply it with or without the assert

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index d8a6cf3..2cc3156 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -2510,8 +2510,12 @@  int av_write_uncoded_frame_query(AVFormatContext *s, int stream_index);
  *
  * May only be called after a successful call to avformat_write_header.
  *
+ * If AVFMT_FLAG_NONBLOCK is set, this call may return AVERROR(EAGAIN)
+ * meaning the operation is pending and the call should be repeated.
+ *
  * @param s media file handle
- * @return 0 if OK, AVERROR_xxx on error
+ * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
+ *         other AVERROR on error
  */
 int av_write_trailer(AVFormatContext *s);
 
diff --git a/libavformat/mux.c b/libavformat/mux.c
index e9973ed..3ae924c 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -1204,11 +1204,14 @@  int av_write_trailer(AVFormatContext *s)
     for (;; ) {
         AVPacket pkt;
         ret = interleave_packet(s, &pkt, NULL, 1);
-        if (ret < 0)
-            goto fail;
         if (!ret)
             break;
 
+        av_assert0(!(s->flags & AVFMT_FLAG_NONBLOCK));
+
+        if (ret < 0)
+            goto fail;
+
         ret = write_packet(s, &pkt);
         if (ret >= 0)
             s->streams[pkt.stream_index]->nb_frames++;
@@ -1238,6 +1241,9 @@  fail:
         }
     }
 
+    if (ret == AVERROR(EAGAIN))
+        return ret;
+
     if (s->oformat->deinit)
         s->oformat->deinit(s);