| Message ID | 1470919109-3327-1-git-send-email-sebechlebskyjan@gmail.com |
|---|---|
| State | New |
| Headers | show |
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 [...]
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
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 [...]
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
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
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 [...]
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);