Re: [FFmpeg-devel] [PATCH] ffmpeg_opt: Add -hwaccels option that lists all supported hwaccels

2015-08-26 Thread Ganesh Ajjanagadde 
On Wed, Aug 26, 2015 at 9:47 PM, Timothy Gu timothyg...@gmail.com wrote:
 On Tue, Aug 25, 2015 at 09:25:14PM -0400, Ganesh Ajjanagadde wrote:
 Ok. Please place the function outside of all the opt_* stuff;
 since this is not setting options, it should not be in the middle of them.
 Otherwise, patch LGTM.

 Pushed with the changes.

Thanks, looks good!


 Timothy
 ___
 ffmpeg-devel mailing list
 ffmpeg-devel@ffmpeg.org
 http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg_opt: Add -hwaccels option that lists all supported hwaccels

2015-08-25 Thread Ganesh Ajjanagadde 
On Tue, Aug 25, 2015 at 7:47 PM, Timothy Gu timothyg...@gmail.com wrote:
 ---
  doc/ffmpeg.texi |  4 
  ffmpeg_opt.c| 14 ++
  2 files changed, 18 insertions(+)

 diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
 index 51a4ec5..e1d8562 100644
 --- a/doc/ffmpeg.texi
 +++ b/doc/ffmpeg.texi
 @@ -698,6 +698,10 @@ is not specified, the value of the @var{DISPLAY} 
 environment variable is used
  For DXVA2, this option should contain the number of the display adapter to 
 use.
  If this option is not specified, the default adapter is used.
  @end table
 +
 +@item -hwaccels
 +List all hardware acceleration methods supported in this build of avconv.
 +

This is ffmpeg, not avconv.

  @end table

  @section Audio Options
 diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
 index a369224..b8b9022 100644
 --- a/ffmpeg_opt.c
 +++ b/ffmpeg_opt.c
 @@ -205,6 +205,18 @@ static int opt_video_standard(void *optctx, const char 
 *opt, const char *arg)
  return opt_default(optctx, standard, arg);
  }

 +static int show_hwaccels(void *optctx, const char *opt, const char *arg)
 +{
 +int i;
 +
 +printf(Hardware acceleration methods:\n);
 +for (i = 0; i  FF_ARRAY_ELEMS(hwaccels) - 1; i++) {
 +printf(%s\n, hwaccels[i].name);
 +}
 +printf(\n);
 +return 0;
 +}

Doesn't this belong in cmdutils.c?
What was your rationale for placing it here?

 +
  static int opt_audio_codec(void *optctx, const char *opt, const char *arg)
  {
  OptionsContext *o = optctx;
 @@ -3241,6 +3253,8 @@ const OptionDef options[] = {
  #if CONFIG_VDA || CONFIG_VIDEOTOOLBOX
  { videotoolbox_pixfmt, HAS_ARG | OPT_STRING | OPT_EXPERT, { 
 videotoolbox_pixfmt},  },
  #endif
 +{ hwaccels, OPT_EXIT,  
 { .func_arg = show_hwaccels },
 +show available HW acceleration methods },

Likewise, shouldn't this be in cmdutils_common_opts.h?
There is no OPT_EXIT option in this table; they are all in
cmdutils_common_opts.h.

  { autorotate,   HAS_ARG | OPT_BOOL | OPT_SPEC |
OPT_EXPERT | OPT_INPUT,
 { .off = OFFSET(autorotate) },
  automatically insert correct rotate filters },
 --
 1.9.1

 ___
 ffmpeg-devel mailing list
 ffmpeg-devel@ffmpeg.org
 http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg_opt: Add -hwaccels option that lists all supported hwaccels

2015-08-25 Thread Timothy Gu 
On Tue, Aug 25, 2015 at 5:06 PM Ganesh Ajjanagadde gajja...@mit.edu wrote:

 On Tue, Aug 25, 2015 at 7:47 PM, Timothy Gu timothyg...@gmail.com wrote:
  ---
   doc/ffmpeg.texi |  4 
   ffmpeg_opt.c| 14 ++
   2 files changed, 18 insertions(+)
 
  diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
  index 51a4ec5..e1d8562 100644
  --- a/doc/ffmpeg.texi
  +++ b/doc/ffmpeg.texi
  @@ -698,6 +698,10 @@ is not specified, the value of the @var{DISPLAY}
 environment variable is used
   For DXVA2, this option should contain the number of the display adapter
 to use.
   If this option is not specified, the default adapter is used.
   @end table
  +
  +@item -hwaccels
  +List all hardware acceleration methods supported in this build of
 avconv.
  +

 This is ffmpeg, not avconv.


Oops. Fixed locally.

Doesn't this belong in cmdutils.c?


No.


 What was your rationale for placing it here?


cmdutils.c is shared for all ff* programs. -hwaccel is only supported in
ffmpeg.

Timothy
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg_opt: Add -hwaccels option that lists all supported hwaccels

2015-08-25 Thread Ganesh Ajjanagadde 
On Tue, Aug 25, 2015 at 7:47 PM, Timothy Gu timothyg...@gmail.com wrote:
 ---
  doc/ffmpeg.texi |  4 
  ffmpeg_opt.c| 14 ++
  2 files changed, 18 insertions(+)

 diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
 index 51a4ec5..e1d8562 100644
 --- a/doc/ffmpeg.texi
 +++ b/doc/ffmpeg.texi
 @@ -698,6 +698,10 @@ is not specified, the value of the @var{DISPLAY} 
 environment variable is used
  For DXVA2, this option should contain the number of the display adapter to 
 use.
  If this option is not specified, the default adapter is used.
  @end table
 +
 +@item -hwaccels
 +List all hardware acceleration methods supported in this build of avconv.
 +
  @end table

  @section Audio Options
 diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
 index a369224..b8b9022 100644
 --- a/ffmpeg_opt.c
 +++ b/ffmpeg_opt.c
 @@ -205,6 +205,18 @@ static int opt_video_standard(void *optctx, const char 
 *opt, const char *arg)
  return opt_default(optctx, standard, arg);
  }

 +static int show_hwaccels(void *optctx, const char *opt, const char *arg)
 +{
 +int i;
 +
 +printf(Hardware acceleration methods:\n);
 +for (i = 0; i  FF_ARRAY_ELEMS(hwaccels) - 1; i++) {
 +printf(%s\n, hwaccels[i].name);
 +}
 +printf(\n);
 +return 0;
 +}

For consistency, prefix with an opt_?
Also, you always return a 0, making return value useless.
I understand this is done to match the func_arg signature.
Maybe you could check the return value of printf?
I do note that almost none of ffmpeg is that meticulous about these things,
but check out:
https://www.irill.org/events/ghm-gnu-hackers-meeting/videos/jim-meyering-goodbye-world-the-perils-of-relying-on-output-streams-in-c

 +
  static int opt_audio_codec(void *optctx, const char *opt, const char *arg)
  {
  OptionsContext *o = optctx;
 @@ -3241,6 +3253,8 @@ const OptionDef options[] = {
  #if CONFIG_VDA || CONFIG_VIDEOTOOLBOX
  { videotoolbox_pixfmt, HAS_ARG | OPT_STRING | OPT_EXPERT, { 
 videotoolbox_pixfmt},  },
  #endif
 +{ hwaccels, OPT_EXIT,  
 { .func_arg = show_hwaccels },
 +show available HW acceleration methods },
  { autorotate,   HAS_ARG | OPT_BOOL | OPT_SPEC |
OPT_EXPERT | OPT_INPUT,
 { .off = OFFSET(autorotate) },
  automatically insert correct rotate filters },
 --
 1.9.1

 ___
 ffmpeg-devel mailing list
 ffmpeg-devel@ffmpeg.org
 http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg_opt: Add -hwaccels option that lists all supported hwaccels

2015-08-25 Thread Ganesh Ajjanagadde 
On Tue, Aug 25, 2015 at 8:08 PM, Timothy Gu timothyg...@gmail.com wrote:
 On Tue, Aug 25, 2015 at 5:06 PM Ganesh Ajjanagadde gajja...@mit.edu wrote:

 On Tue, Aug 25, 2015 at 7:47 PM, Timothy Gu timothyg...@gmail.com wrote:
  ---
   doc/ffmpeg.texi |  4 
   ffmpeg_opt.c| 14 ++
   2 files changed, 18 insertions(+)
 
  diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
  index 51a4ec5..e1d8562 100644
  --- a/doc/ffmpeg.texi
  +++ b/doc/ffmpeg.texi
  @@ -698,6 +698,10 @@ is not specified, the value of the @var{DISPLAY}
 environment variable is used
   For DXVA2, this option should contain the number of the display adapter
 to use.
   If this option is not specified, the default adapter is used.
   @end table
  +
  +@item -hwaccels
  +List all hardware acceleration methods supported in this build of
 avconv.
  +

 This is ffmpeg, not avconv.


 Oops. Fixed locally.

 Doesn't this belong in cmdutils.c?


 No.


 What was your rationale for placing it here?


 cmdutils.c is shared for all ff* programs. -hwaccel is only supported in
 ffmpeg.

Why?
I don't know about this and I might be naive here,
but isn't hardware acceleration useful for decoding as well?
In this case it would be relevant to at least ffplay as well as ffmpeg.


 Timothy
 ___
 ffmpeg-devel mailing list
 ffmpeg-devel@ffmpeg.org
 http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg_opt: Add -hwaccels option that lists all supported hwaccels

2015-08-25 Thread Timothy Gu 
On Tue, Aug 25, 2015 at 08:19:59PM -0400, Ganesh Ajjanagadde wrote:
 For consistency, prefix with an opt_?

This name is chosen to be consistent with other show_ functions. opt_*
are exclusively used for setting some options AFAICT.

 Maybe you could check the return value of printf?

Absolutely zero calls to printf in FFmpeg are checked. Even if checking
the result code can be more secure, it is way outside the scope of this
patch.

Timothy
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg_opt: Add -hwaccels option that lists all supported hwaccels

2015-08-25 Thread Ganesh Ajjanagadde 
On Tue, Aug 25, 2015 at 9:03 PM, Timothy Gu timothyg...@gmail.com wrote:
 On Tue, Aug 25, 2015 at 08:19:59PM -0400, Ganesh Ajjanagadde wrote:
 For consistency, prefix with an opt_?

 This name is chosen to be consistent with other show_ functions. opt_*
 are exclusively used for setting some options AFAICT.

Ok. Please place the function outside of all the opt_* stuff;
since this is not setting options, it should not be in the middle of them.
Otherwise, patch LGTM.


 Maybe you could check the return value of printf?

 Absolutely zero calls to printf in FFmpeg are checked. Even if checking
 the result code can be more secure, it is way outside the scope of this
 patch.

Perhaps. My point was not so much from the position of security,
but program correctness: a non zero printf return should propagate to
the exit code.
You are right this is beyond this patch's scope.


 Timothy
 ___
 ffmpeg-devel mailing list
 ffmpeg-devel@ffmpeg.org
 http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel