| Message ID | E8CA02F7-7F3C-4D0A-BAFC-24CAB8A57AEB@mac.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Sep 10, 2016 at 01:21:10PM +0300, Oliver Collyer wrote: > > >> I have written a patch for FFmpeg that deals with the problem for both > >> devices so it’s not really an issue for me anymore, but I’m not sure > >> if the patch will get accepted in their master git as it’s a little > >> messy. > > > > Please post this patch here! > > Here you go, Andrey. This patch basically makes it throw away corrupted buffers and then also the first 8 buffers after the last corrupted buffer. Thanks a lot for sharing. > It’s not sufficient just to throw away the corrupted buffers as I have noticed that the first few legitimate buffers appear at slightly irregular time intervals leading to FFmpeg spewing out a bunch of warnings for the duration of the capture. In my tests around 3 buffers have to be ignored but I’ve fixed it at 8 to be on the safe side. It’s a bit ugly though, to be honest, I don’t know how the number of buffers that need to be ignored would depend on the framerate, video size etc, but it works for my 1080i test. > > With this patch, you get some warnings at the start, for both devices, as it encounters (and recovers from) the corrupted buffers but after that the captures work just fine. > > > diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c > old mode 100644 > new mode 100755 > index ddf331d..7b4a826 > --- a/libavdevice/v4l2.c > +++ b/libavdevice/v4l2.c > @@ -79,6 +79,7 @@ struct video_data { > > int buffers; > volatile int buffers_queued; > + int buffers_ignore; > void **buf_start; > unsigned int *buf_len; > char *standard; > @@ -519,7 +520,9 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt) > av_log(ctx, AV_LOG_WARNING, > "Dequeued v4l2 buffer contains corrupted data (%d bytes).\n", > buf.bytesused); > - buf.bytesused = 0; > + s->buffers_ignore = 8; > + enqueue_buffer(s, &buf); > + return FFERROR_REDO; > } else > #endif > { > @@ -529,14 +532,28 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt) > s->frame_size = buf.bytesused; > > if (s->frame_size > 0 && buf.bytesused != s->frame_size) { > - av_log(ctx, AV_LOG_ERROR, > + av_log(ctx, AV_LOG_WARNING, > "Dequeued v4l2 buffer contains %d bytes, but %d were expected. Flags: 0x%08X.\n", > buf.bytesused, s->frame_size, buf.flags); > + s->buffers_ignore = 8; > enqueue_buffer(s, &buf); > - return AVERROR_INVALIDDATA; > + return FFERROR_REDO; > } > } These two chunks look like legit resilience measure, and maybe could be even added to upstream ffmpeg, maybe for non-default mode. > > + > + /* if we just encounted some corrupted buffers then we ignore the next few > + * legitimate buffers because they can arrive at irregular intervals, causing > + * the timestamps of the input and output streams to be out-of-sync and FFmpeg > + * to continually emit warnings. */ > + if (s->buffers_ignore) { > + av_log(ctx, AV_LOG_WARNING, > + "Ignoring dequeued v4l2 buffer due to earlier corruption.\n"); > + s->buffers_ignore --; > + enqueue_buffer(s, &buf); > + return FFERROR_REDO; > + } Not clear exactly happens here so that such workaround is needed... Congratulations, you've ended up with a workaround which works for you, for such a mysterious issue :) I still don't know what exactly causes this error condition on original layer (I suppose that's some "panic" in the peripheral device), but I guess that due to rarity of this condition, V4L2 code developers (in both kernel and ffmpeg) just haven't had an opportunity to debug such situations and handled this error condition formally, without experience of running into it, and knowledge why it happens and how it could be handled in most resilient way. (Maybe this should NOT be handled in resilient way in theory, but still works for your case.) So you had to pave your own way here. Maybe comments from senior V4L2 developers shed more lights on this. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > These two chunks look like legit resilience measure, and maybe could be > even added to upstream ffmpeg, maybe for non-default mode. > Well I’ve posted it to the FFmpeg dev list for feedback, so we will see. Non-default mode - yes maybe it needs to be optional. And/or only have an effect at the start of the capture; I am concerned that in some situation where a capture momentarily loses signal and delivers a corrupted buffer that my patch would then actually do more than an end user would require by ignoring subsequent buffers and maybe turning it into a bigger issue. >> >> + >> + /* if we just encounted some corrupted buffers then we ignore the next few >> + * legitimate buffers because they can arrive at irregular intervals, causing >> + * the timestamps of the input and output streams to be out-of-sync and FFmpeg >> + * to continually emit warnings. */ >> + if (s->buffers_ignore) { >> + av_log(ctx, AV_LOG_WARNING, >> + "Ignoring dequeued v4l2 buffer due to earlier corruption.\n"); >> + s->buffers_ignore --; >> + enqueue_buffer(s, &buf); >> + return FFERROR_REDO; >> + } > > Not clear exactly happens here so that such workaround is needed… > Yes, this is the ugly bit. I had it outputting the timestamps of all the buffers received and it clearly showed that 2-3 of them are stamped closer together. It’s as if something is taking extra time to recover from whatever was causing the original problem. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c old mode 100644 new mode 100755 index ddf331d..7b4a826 --- a/libavdevice/v4l2.c +++ b/libavdevice/v4l2.c @@ -79,6 +79,7 @@ struct video_data { int buffers; volatile int buffers_queued; + int buffers_ignore; void **buf_start; unsigned int *buf_len; char *standard; @@ -519,7 +520,9 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt) av_log(ctx, AV_LOG_WARNING, "Dequeued v4l2 buffer contains corrupted data (%d bytes).\n", buf.bytesused); - buf.bytesused = 0; + s->buffers_ignore = 8; + enqueue_buffer(s, &buf); + return FFERROR_REDO; } else #endif { @@ -529,14 +532,28 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt) s->frame_size = buf.bytesused; if (s->frame_size > 0 && buf.bytesused != s->frame_size) { - av_log(ctx, AV_LOG_ERROR, + av_log(ctx, AV_LOG_WARNING, "Dequeued v4l2 buffer contains %d bytes, but %d were expected. Flags: 0x%08X.\n", buf.bytesused, s->frame_size, buf.flags); + s->buffers_ignore = 8; enqueue_buffer(s, &buf); - return AVERROR_INVALIDDATA; + return FFERROR_REDO; } } + + /* if we just encounted some corrupted buffers then we ignore the next few + * legitimate buffers because they can arrive at irregular intervals, causing + * the timestamps of the input and output streams to be out-of-sync and FFmpeg + * to continually emit warnings. */ + if (s->buffers_ignore) { + av_log(ctx, AV_LOG_WARNING, + "Ignoring dequeued v4l2 buffer due to earlier corruption.\n"); + s->buffers_ignore --; + enqueue_buffer(s, &buf); + return FFERROR_REDO; + } + /* Image is at s->buff_start[buf.index] */ if (avpriv_atomic_int_get(&s->buffers_queued) == FFMAX(s->buffers / 8, 1)) { /* when we start getting low on queued buffers, fall back on copying data */ @@ -608,6 +625,7 @@ static int mmap_start(AVFormatContext *ctx) } } s->buffers_queued = s->buffers; + s->buffers_ignore = 0; type = V4L2_BUF_TYPE_VIDEO_CAPTURE; if (v4l2_ioctl(s->fd, VIDIOC_STREAMON, &type) < 0) {