[MPlayer-dev-eng] [PATCH] Capture feature

[Reimar Döffinger]

On Sun, Sep 05, 2010 at 02:01:34PM +0200, Pásztor Szilárd wrote:
> Reimar Döffinger:
> > Sorry that I was unclear. I didn't mean for you to implement
> > this feature, I was just explaining why it is better to have it in the stream struct.
> > This approach ends up with more global variables, I was rather
> > hoping to keep the stream layer clean from that.
> > I don't know if/how this is possible, but my idea was for the
> > command.c code to open/close the file and set the FILE *
> > within the stream appropriately, and not having any logic
> > beyond if (capture_file) fwrite(...);
> > in the stream layer...
> 
> Here I attach a simpler version where only the primary stream
> gets dumped again (it doesn't make much sense to make things
> more complicated than that). No bumming global variables and
> no extensive logic in the stream beyond writing.

Please send patches with some more appropriate mime type, e.g. 
text/plain, this makes reviewing and answering much simpler.

> void stream_capture_do(stream_t *s, int len);
The len argument is always s->buf_len, so it seems rather pointless.

> +               static int capturing = 0;
> +               if (capture_dump_name)
> +                   capturing ^= 1;
> +               else
> +                   break;

Maybe a failure message even in that case would be good?

> +               if (capturing) {
> +                   if (mpctx->stream->capture_file) {
> +                       // shouldn't happen

Actually this will happen all the time, e.g. when switching
to the next file while capturing is on.
At least some of this inconsistency could be avoided if you
just always used mpctx->stream->capture_file instead of a
separate capturing variable.
If this was implemented as a property, users would also have the choice
between using this as a toggle or have separate keys for on and off.
It would also allow a proper return value in slave mode when this
is not available (which should be used if mpctx->stream is NULL as e.g.
with -idle, instead of crashing which I think your code currently does).