[PATCH] lavf: AVFMT_HAS_NONBLOCK flag for (de)muxers
Hi, This patch introduces a new flag to indicate whether a format supports non-blocking reading and/or writing. Not sure if micro or minor version number had to be changed. Searching back on the mailinglist gave no conclusive answer. The same goes for the addition to APIchanges. Are the rules about that written down somewhere? Regards, --Ivo
Hi,
On Thu, 2009-11-26 at 18:08 +0100, Ivo wrote:
> On Thursday 26 November 2009, 18:06:33, Ivo wrote:
> > This patch...
[...]
> while((res = snd_pcm_writei(s->h, buf, size / s->frame_size)) < 0) {
> - if (res == -EAGAIN) {
> + if (res == -EAGAIN && s1->flags & AVFMT_FLAG_NONBLOCK) {
>
> return AVERROR(EAGAIN);
> }
Are you sure about this hunk? I would not expect alsa to return -EAGAIN
if the non-blocking flag has not been set previously...
Also, I think you said that the OSS driver is not honouring the NONBLOCK
flag... (or am I misremembering?). If this is the case, I suspect that
simply adding AVFMT_HAS_NONBLOCK to the oss_demuxer flags is not enough.
Or am I missing something?
Thanks,
Luca
On Thursday 26 November 2009, 21:10:35, Luca Abeni wrote:
On Thu, 2009-11-26 at 18:08 +0100, Ivo wrote:
On Thursday 26 November 2009, 18:06:33, Ivo wrote:
This patch...
[...]
while((res = snd_pcm_writei(s->h, buf, size / s->frame_size)) < 0) { - if (res == -EAGAIN) { + if (res == -EAGAIN && s1->flags & AVFMT_FLAG_NONBLOCK) {
return AVERROR(EAGAIN); }
Are you sure about this hunk? I would not expect alsa to return -EAGAIN if the non-blocking flag has not been set previously...
I checked the Alsa API docs and -EGAIN isn't a valid return value in any case: [snd_pcm_writei()] Returns: a positive number of frames actually written otherwise a negative error code [negative] Return values: -EBADFD, -EPIPE, -ESTRPIPE So, basically, the code is incorrect. Hopefully, alsa supports a way to check whether writing x frames will block, so we can return EAGAIN in that case, otherwise I think a non-blocking alsa muxer is not possible. I'll look into it.
Also, I think you said that the OSS driver is not honouring the NONBLOCK flag... (or am I misremembering?). If this is the case, I suspect that simply adding AVFMT_HAS_NONBLOCK to the oss_demuxer flags is not enough. Or am I missing something?
I have checked the code and the OSS demuxer returns an empty packet (pkt->size=0; pkt->pts=av_gettime();) if it's in non-blocking mode. In blocking mode, it actually does the same if the select() times out. The OSS muxer always blocks, hence I did not add the flag there. Regards, --Ivo
Ivo wrote: [...]
while((res = snd_pcm_writei(s->h, buf, size / s->frame_size)) < 0) { - if (res == -EAGAIN) { + if (res == -EAGAIN && s1->flags & AVFMT_FLAG_NONBLOCK) {
return AVERROR(EAGAIN); }
Are you sure about this hunk? I would not expect alsa to return -EAGAIN if the non-blocking flag has not been set previously...
I checked the Alsa API docs and -EGAIN isn't a valid return value in any case:
[snd_pcm_writei()] Returns: a positive number of frames actually written otherwise a negative error code
[negative] Return values: -EBADFD, -EPIPE, -ESTRPIPE
This funny... Ok; since it is ALSA, everything is possible ;-) Which kind of error is returned if a non-blocking write fails because the output buffer is full?
Also, I think you said that the OSS driver is not honouring the NONBLOCK flag... (or am I misremembering?). If this is the case, I suspect that simply adding AVFMT_HAS_NONBLOCK to the oss_demuxer flags is not enough. Or am I missing something?
I have checked the code and the OSS demuxer returns an empty packet (pkt->size=0; pkt->pts=av_gettime();) if it's in non-blocking mode. In blocking mode, it actually does the same if the select() times out.
The OSS muxer always blocks, hence I did not add the flag there.
Ah, sorry... I misunderstood some previous email. Thanks, Luca
On Thursday 26 November 2009, 21:10:35, Luca Abeni wrote:
On Thu, 2009-11-26 at 18:08 +0100, Ivo wrote:
On Thursday 26 November 2009, 18:06:33, Ivo wrote:
This patch...
[...]
while((res = snd_pcm_writei(s->h, buf, size / s->frame_size)) < 0) { - if (res == -EAGAIN) { + if (res == -EAGAIN && s1->flags & AVFMT_FLAG_NONBLOCK) {
return AVERROR(EAGAIN); }
Are you sure about this hunk? I would not expect alsa to return -EAGAIN if the non-blocking flag has not been set previously...
I dropped this hunk locally. Non-blocking mode is non-blocking, but it does not behave entirely correctly. It should not check for -EAGAIN, but use snd_pcm_status_get_avail upfront, before writing the frames. If there's not enough room, it should return -EGAIN, as there's no way to signal the calling application that some frames were not written. But such fix belongs in a separate patch IMHO. So, I dropped this hunk and the rest is the same. If it's ok to apply, I'll increase the micro number and update APIchanges in a separate commit. Regards, --Ivo
Ivo wrote: [...]
I dropped this hunk locally. Non-blocking mode is non-blocking, but it does not behave entirely correctly. It should not check for -EAGAIN, but use snd_pcm_status_get_avail upfront, before writing the frames.
Yes, I just ran some tests, and I verified that if the check is not performed ALSA writes some bytes and snd_pcm_writei() returns a very small positive value...
If there's not enough room, it should return -EGAIN, as there's no way to signal the calling application that some frames were not written. But such fix belongs in a separate patch IMHO.
I agree.
So, I dropped this hunk and the rest is the same. If it's ok to apply, I'll increase the micro number and update APIchanges in a separate commit.
Since nonblocking ALSA output is currently buggy, I am not sure if adding AVFMT_HAS_NONBLOCK in alsa_muxer is a good idea or not. Maybe it can be added in the future patch that fixes nonblocking alsa out? But I have no strong opinions about this. Apart from this issue, I now think it's fine. If Michael is ok with the patch, I'd say commit it. Thanks, Luca
On Friday 27 November 2009, 16:06:46, Luca Abeni wrote:
Ivo wrote:
So, I dropped this hunk and the rest is the same. If it's ok to apply, I'll increase the micro number and update APIchanges in a separate commit.
Since nonblocking ALSA output is currently buggy, I am not sure if adding AVFMT_HAS_NONBLOCK in alsa_muxer is a good idea or not. Maybe it can be added in the future patch that fixes nonblocking alsa out? But I have no strong opinions about this.
I agree. I won't add it for the muxer yet.
Apart from this issue, I now think it's fine. If Michael is ok with the patch, I'd say commit it.
Regards, --Ivo
On Fri, Nov 27, 2009 at 04:06:46PM +0100, Luca Abeni wrote: [...]
Apart from this issue, I now think it's fine. If Michael is ok with the patch,
iam not alsa maintainer [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras
On Fri, 2009-11-27 at 19:48 +0100, Michael Niedermayer wrote:
On Fri, Nov 27, 2009 at 04:06:46PM +0100, Luca Abeni wrote: [...]
Apart from this issue, I now think it's fine. If Michael is ok with the patch,
iam not alsa maintainer
I think we have no ALSA maintainer, so I was applying the standard "fallback to Michael" thing ;-) Anyway, if you are ok with the "generic" (avformat.h) part of the patch I think it's worth applying it (maybe waiting one day or two, to give other people to possibility to comment). Thanks, Luca
On Fri, Nov 27, 2009 at 08:00:11PM +0100, Luca Abeni wrote:
On Fri, 2009-11-27 at 19:48 +0100, Michael Niedermayer wrote:
On Fri, Nov 27, 2009 at 04:06:46PM +0100, Luca Abeni wrote: [...]
Apart from this issue, I now think it's fine. If Michael is ok with the patch,
iam not alsa maintainer
I think we have no ALSA maintainer, so I was applying the standard "fallback to Michael" thing ;-)
well ask for volunteers and if you find none threaten with svn rm alsa if that still produces no volunteer maintainer, it probably means noone cares about alsa and it can saftely be removed.
Anyway, if you are ok with the "generic" (avformat.h) part of the patch I think it's worth applying it (maybe waiting one day or two, to give other people to possibility to comment).
iam ok with the generic part, but i would suspect that at least av_interleaved_write_frame() needs changes to be able to recover from a EAGAIN without loosing packets [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator
On Friday 27 November 2009, 20:23:08, Michael Niedermayer wrote:
On Fri, Nov 27, 2009 at 08:00:11PM +0100, Luca Abeni wrote:
I think we have no ALSA maintainer, so I was applying the standard "fallback to Michael" thing ;-)
well ask for volunteers and if you find none threaten with svn rm alsa if that still produces no volunteer maintainer, it probably means noone cares about alsa and it can saftely be removed.
Luca, didn't you write the alsa backend with Benoit? Or did you drop maintainership later?
Anyway, if you are ok with the "generic" (avformat.h) part of the patch I think it's worth applying it (maybe waiting one day or two, to give other people to possibility to comment).
iam ok with the generic part, but i would suspect that at least av_interleaved_write_frame() needs changes to be able to recover from a EAGAIN without loosing packets
I looked at av_interleaved_write_frame and it passes -EGAIN on to the calling application. I have alsa fixed locally and it's ffmpeg.c that does this if alsa receives the packages too fast in non-blocking mode: ret= av_interleaved_write_frame(s, pkt); if(ret < 0){ print_error("av_interleaved_write_frame()", ret); av_exit(1); } Shouldn't the calling application act on -EAGAIN, wait a little and try av_interleaved_write_frame(s,pkt) again? Or will that result in duplicate packets? (the code is not entirely clear to me yet) --Ivo
On Sun, Nov 29, 2009 at 11:46:58PM +0100, Ivo wrote:
On Friday 27 November 2009, 20:23:08, Michael Niedermayer wrote:
On Fri, Nov 27, 2009 at 08:00:11PM +0100, Luca Abeni wrote: [...]
Anyway, if you are ok with the "generic" (avformat.h) part of the patch I think it's worth applying it (maybe waiting one day or two, to give other people to possibility to comment).
iam ok with the generic part, but i would suspect that at least av_interleaved_write_frame() needs changes to be able to recover from a EAGAIN without loosing packets
I looked at av_interleaved_write_frame and it passes -EGAIN on to the calling application. I have alsa fixed locally and it's ffmpeg.c that does this if alsa receives the packages too fast in non-blocking mode:
ret= av_interleaved_write_frame(s, pkt); if(ret < 0){ print_error("av_interleaved_write_frame()", ret); av_exit(1); }
Shouldn't the calling application act on -EAGAIN, wait a little and try av_interleaved_write_frame(s,pkt) again?
yes, though not just wait but rather try the next "file" if there are more.
Or will that result in duplicate packets? (the code is not entirely clear to me yet)
as (unclearly) said, av_interleaved_write_frame() or related code might need some change [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better.
Ivo wrote:
On Friday 27 November 2009, 20:23:08, Michael Niedermayer wrote:
On Fri, Nov 27, 2009 at 08:00:11PM +0100, Luca Abeni wrote:
I think we have no ALSA maintainer, so I was applying the standard "fallback to Michael" thing ;-) well ask for volunteers and if you find none threaten with svn rm alsa if that still produces no volunteer maintainer, it probably means noone cares about alsa and it can saftely be removed.
Luca, didn't you write the alsa backend with Benoit? Or did you drop maintainership later?
If I remember well, I originally wrote it (with contributions from Benoit), but Nicolas George did all the work for making it suitable for svn and having it committed. Anyway, if (and only if :) noone else is available for maintaining the alsa muxer and demuxer, and if the absence of maintainer can really cause the removal from svn, then I'll "volunteer" as a maintainer ;-) Luca
participants (3)
-
ivop＠euronet.nl -
lucabe72＠email.it -
michaelni＠gmx.at