[FFmpeg-devel] [PATCH 1/2] file: operate in blocks, check for interrupts

[Andrey Utkin]

2013/7/10 Michael Niedermayer <michaelni at gmx.at>:
> This probably makes most sense
> extending the callback is tricky though due to AVIOInterruptCB not
> having any flags and being non extensible.
> The cleanest solution i see ATM would be adding a special case
> something like
> if(cb->callback == CHECK_INT && *(int*)cb->callback->opaque > 0)
>     return AVERROR_EXIT
>
> This does no callback at all but just checks an integer from a user
> provided pointer so its not affected by the user mistakely providing
> a slow callback. And this kind of check could then be done in the
> retry_transfer_wrapper loop

I no longer think that such optimization is a good idea.
The rationale (possible slow callbacks) is very questionable.
I have even sat to code it, but then got confused.
Now retry_interrupt_wrapper() works this way

while (len < size_min) {
    /* try transfer */
    ff_check_interrupt();
}

With what you proposed, it would look this way

while (len < size_min) {
    /* check interrupt in special case, using AVIOInterruptCB
directly, which is ugly by itself */
    /* try transfer */
    /* check interrupt */
}

This makes things complicated, and the reason is supporting slow
callbacks - while being unsure if they exist.
This is just one procedure which calls ff_check_interrupt() in a loop,
there are more procedures which check for interrupt _not_ in first
place (at last ff_network_wait_fd_timeout()). Should we consistently
add second special check everywhere where applicable? It is hard to
believe/understand/explain why this technique would be needed.
It seems to me now that simpler and cleaner resolution is just to move
interrupt check to the beginning to loop body in
retry_transfer_wrapper. After all this callback is the tradeoff for
interruptibility, and application developer must understand it. If
such check is not needed in specific case, (i.e. file is known to be
accessed reasonably fast), app developer may set no callback at all,
or set it for (de)muxer only.

The idea of special value for callback is nice. The patch for it may
look like what i attach.
Although this patch is not essential for solution of discussed
problem, so it is not necessary to apply immediately. (For the
addressed problem, i think more important is the policy to check for
interrupt _before_ any blocking operations, and introducing local
files reading in blocks.)

> Also ffplay & ffmpeg just check a pointed to variable so they could
> easily use it.
>
> I wonder if (m)any applications do more complex things than checking
> a variable in the callback.

I have also never seen such cases. Mostly none callback is used, or
just flag checking is done.
I guess very few _may_ do that, and IMO calling their callback more
times cannot be treated a regression.
I'd say that libavformat is free to check the callback whenever it
considers it reasonable, and that is OK for application developer.

--
Andrey Utkin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Special-case-for-ff_check_interrupt.patch
Type: application/octet-stream
Size: 2011 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130710/a60e9348/attachment.obj>