[MPlayer-dev-eng] [PATCH] vf_screenshot, output filename corresponding to the played file

[Ruslan Savchenko]

On Tue, Nov 25, 2008 at 10:44 PM, Uoti Urpala <uoti.urpala at pp1.inet.fi> wrote:
> On Sun, 2008-11-23 at 19:59 +0300, Ruslan Savchenko wrote:
>> All attached patches except of 0001 and 0008 refer to vf_screenshot.c.
>> Here is a brief explanation about each patch:
>>
>>  0001-Move-filename-variable-to-global-in-mencoder.c.patch
>>     Move 'filename' variable outside main() to global in mencoder.c.
>
> I still think this is a bad idea. A filter shouldn't rely on such
> globals existing. What if the "file" is an URL? Standard input? You'd
> need some kind of policy for naming the output, and it does not really
> belong in a filter.

If "://" appears in the global filename, then "shot" is used, not filename.

>
> If functionality like this is really required then I think there should
> be top-level code deciding what subdirectory or prefix to use for all
> output related to a file, whatever filter or other part generates it.

I'll look into such solution, but I think the design (at least idea)
should be done by more experienced people than me.

>
>> In mplayer.c 'filename' is already global.
>
> I've already removed that global in my git tree.
>
>
>> 0002-Replace-magic-number-102-by-SHOT_FNAME_LENGTH-macro.patch
>>     Define SHOT_FNAME_LENGTH and use it instead of magic numbers.
>>
>> 0003-Change-priv-fname-allocation-from-static-to-dynamic.patch
>>     Allocate priv->fname dynamically in gen_fname() and free it in
>> put_image(). Dynamic allocation is required by further patches.
>
> Seems the filename is only used immediately after being generated and
> there is no need to allocate permanent storage for it. It should be
> possible to store it in a local variable with much simpler code.
>

Yes, but the question is what length should be priv->fname? I used 256
in very the first version of vf_screenshot patch but it seemed this
number didn't satisfy everyone. Even I once had a video file with file
name length more than 256 chars.

-- 
Regards,
savrus