[FFmpeg-devel] [PATCH] set FONTCONFIG_PATH relative to ffmpeg binary, if it is a static build

[Nicolas George]

Le nonidi 29 pluviôse, an CCXXII, Helmut K. C. Tessarek a écrit :
> For static builds FONTCONFIG might not be installed on the system, so ffmpeg will throw an error.
> Also, if people bundled the binary, a relative fontconfig path would be an advantage.
> ---
>  ffmpeg.c | 31 +++++++++++++++++++++++++++++++
>  ffmpeg.h |  1 +
>  2 files changed, 32 insertions(+)

Thanks for the patch. It requires quite a few adjustments before being
considered.

First, please try to follow the coding style of the surrounding code. The
most obvious problem is the tabs, which are completely forbidden in the code
base, but there are other:
- no spaces before or inside parentheses for function arguments;
- no spaces inside but one before parentheses for C keywords ("if", "for");
- for blocks (but not functions), braces on the same line that the
  corresponding keyword (i.e.: "if (...) {", no \n before '{').

Other more specific remarks interspersed in the code.

> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 6a51810..d8f9443 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -3541,11 +3541,42 @@ static void log_callback_null(void *ptr, int level, const char *fmt, va_list vl)
>  {
>  }
>  
> +void set_relative_fontconfig_path(void)
> +{
> +#if !HAVE_WINDOWS_H
> +	char subdir[] = "/fonts";

> +#else
> +	char subdir[] = "\\fonts";
> +#endif

Is it necessary? I thought that windows understood normal /-separated path
names.

> +
> +	char *curdir = (char *) av_malloc (PATH_MAX);
> +	char *fontdir = (char *) av_malloc (PATH_MAX);

Casting the result of malloc is a c++ aberration. And you need to check for
memory allocation failure.

> +
> +	getcwd (curdir, PATH_MAX);

Is it necessary? A quick test shows that fontconfig can handle relative
paths just fine.

And checking for failure is missing.

> +
> +	strncpy( fontdir, curdir, PATH_MAX );

IIRC, strncpy() is braindead, rather use av_strlcpy(). But what is the point
of doing it in two steps?

> +	fontdir[PATH_MAX-1] = '\0';
> +
> +	if( strlen(curdir) < (PATH_MAX - 7) )
> +	{
> +		strncat( fontdir, subdir, 7 );
> +	}

av_strlcat(). And what happens if the condition is not met?

> +
> +	setenv( "FONTCONFIG_PATH", fontdir, 0 );

This will overwrite the environment variable if it exists. The user-provided
value should have precedence.

> +
> +	av_free( fontdir );
> +	av_free( curdir );
> +}
> +
>  int main(int argc, char **argv)
>  {
>      int ret;
>      int64_t ti;
>  

> +#if CONFIG_STATIC && CONFIG_FONTCONFIG

This looks wrong: CONFIG_STATIC tells you that the FFmpeg libraries will be
built static; they may also be built shared: CONFIG_STATIC and CONFIG_SHARED
are not exclusive.

It does not tell you that fontconfig will be statically linked.

> +    set_relative_fontconfig_path();
> +#endif
> +
>      register_exit(ffmpeg_cleanup);
>  
>      setvbuf(stderr,NULL,_IONBF,0); /* win32 runtime needs this */
> diff --git a/ffmpeg.h b/ffmpeg.h
> index 00f7a2a..48c6451 100644
> --- a/ffmpeg.h
> +++ b/ffmpeg.h
> @@ -489,6 +489,7 @@ int configure_output_filter(FilterGraph *fg, OutputFilter *ofilter, AVFilterInOu
>  int ist_in_filtergraph(FilterGraph *fg, InputStream *ist);
>  FilterGraph *init_simple_filtergraph(InputStream *ist, OutputStream *ost);
>  

> +void set_relative_fontconfig_path(void);

This function is only used inside ffmpeg.c: please make it static.

>  int ffmpeg_parse_options(int argc, char **argv);
>  
>  int vdpau_init(AVCodecContext *s);

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140217/31e9aeac/attachment.asc>