Re: [FFmpeg-devel] [PATCH 03/11] ffserver_config: map ffserver options to AVOptions
Hi
On 11/21/2014 09:16 PM, Lukasz Marek wrote:
[...]
@@ -497,6 +500,14 @@ static int ffserver_save_avoption(const char *opt, const
char *arg, int type, FF
return 0;
}
+static int ffserver_save_avoption_int(const char *opt, int64_t arg,
+ int type, FFServerConfig *config)
+{
+char buf[30];
char buf[20] should be plenty considering INT64_MAX. Feel free to push
after this minor fix unless someone else holds you.
Bests,
--
Reynaldo H. Verdejo Pinochet
Open Source Group
Samsung Research America / Silicon Valley
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 03/11] ffserver_config: map ffserver options to AVOptions
On 26.11.2014 23:22, Reynaldo H. Verdejo Pinochet wrote:
Hi
On 11/21/2014 09:16 PM, Lukasz Marek wrote:
[...]
@@ -497,6 +500,14 @@ static int ffserver_save_avoption(const char *opt, const
char *arg, int type, FF
return 0;
}
+static int ffserver_save_avoption_int(const char *opt, int64_t arg,
+ int type, FFServerConfig *config)
+{
+char buf[30];
char buf[20] should be plenty considering INT64_MAX. Feel free to push
after this minor fix unless someone else holds you.
In fact 20 is too small. INT64_MIN has 19 digits, '-' sign and
terminating 0. So 21 is a minimum. I put 22, the same as av_dict_set_int.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 03/11] ffserver_config: map ffserver options to AVOptions
On 11/26/2014 07:52 PM, Lukasz Marek wrote: [..] In fact 20 is too small. INT64_MIN has 19 digits, '-' sign and terminating 0. So 21 is a minimum. I put 22, the same as av_dict_set_int. True, forgot about the sign. Go ahead after fixing please. Thanks, -- Reynaldo H. Verdejo Pinochet Open Source Group Samsung Research America / Silicon Valley ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 03/11] ffserver_config: map ffserver options to AVOptions
Hi On 11/20/2014 10:14 PM, Lukasz Marek wrote: On 18.11.2014 21:35, Reynaldo H. Verdejo Pinochet wrote: [...] Also, please make an effort to break lines at 80 chars as long as it doesn't make the code harder to read. This seems particularly possible on the function declarations. regarding this part, I set my editor to notice 90 chars and I try to respect that, but with some reasonable margin. TBH, 80 chars is prehistoric, probably from the era of crt's with text mode. I don't remember official ffmpeg's rules about that, but common, its 21th century... https://www.ffmpeg.org/developer.html#toc-Code-formatting-conventions Please try to when possible. It's not a requirement but a sane suggestion many avid by. Bests, -- Reynaldo H. Verdejo Pinochet Open Source Group Samsung Research America / Silicon Valley ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 03/11] ffserver_config: map ffserver options to AVOptions
On 11/21/2014 07:17 PM, Reynaldo H. Verdejo Pinochet wrote: [..] suggestion many avid by. s/avid by/follow/g ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 03/11] ffserver_config: map ffserver options to AVOptions
On 18.11.2014 21:35, Reynaldo H. Verdejo Pinochet wrote:
Hi
On 11/16/2014 10:46 PM, Lukasz Marek wrote:
Signed-off-by: Lukasz Marek lukasz.m.lu...@gmail.com
[..]
@@ -965,43 +881,38 @@ static int ffserver_parse_config_stream(FFServerConfig
*config, const char *cmd,
ret = av_parse_video_size(w, h, arg);
if (ret 0)
ERROR(Invalid video size '%s'\n, arg);
-else if ((w % 2) || (h % 2))
See bellow
-WARNING(Image size is not a multiple of 2\n);
-if (av_dict_set_int(config-video_conf, VideoSizeWidth, w, 0) 0 ||
-av_dict_set_int(config-video_conf, VideoSizeHeight, h, 0) 0)
-goto nomem;
-} else if (!av_strcasecmp(cmd, VideoFrameRate)) {
-AVRational frame_rate;
-ffserver_get_arg(arg, sizeof(arg), p);
-if (av_parse_video_rate(frame_rate, arg) 0) {
-ERROR(Incorrect frame rate: %s\n, arg);
-} else {
-if (av_dict_set_int(config-video_conf, VideoFrameRateNum,
frame_rate.num, 0) 0 ||
-av_dict_set_int(config-video_conf, VideoFrameRateDen,
frame_rate.den, 0) 0)
+else {
+if ((w % 2) || (h % 2))
Drop the redundant () across %.
This part is not part of the commit, but I will remove.
Also, please make an effort
to break lines at 80 chars as long as it doesn't make the
code harder to read. This seems particularly possible on the
function declarations.
regarding this part, I set my editor to notice 90 chars and I try to
respect that, but with some reasonable margin. TBH, 80 chars is
prehistoric, probably from the era of crt's with text mode.
I don't remember official ffmpeg's rules about that, but common, its
21th century...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 03/11] ffserver_config: map ffserver options to AVOptions
Hi
On 11/16/2014 10:46 PM, Lukasz Marek wrote:
Signed-off-by: Lukasz Marek lukasz.m.lu...@gmail.com
[..]
@@ -965,43 +881,38 @@ static int ffserver_parse_config_stream(FFServerConfig
*config, const char *cmd,
ret = av_parse_video_size(w, h, arg);
if (ret 0)
ERROR(Invalid video size '%s'\n, arg);
-else if ((w % 2) || (h % 2))
See bellow
-WARNING(Image size is not a multiple of 2\n);
-if (av_dict_set_int(config-video_conf, VideoSizeWidth, w, 0) 0
||
-av_dict_set_int(config-video_conf, VideoSizeHeight, h, 0)
0)
-goto nomem;
-} else if (!av_strcasecmp(cmd, VideoFrameRate)) {
-AVRational frame_rate;
-ffserver_get_arg(arg, sizeof(arg), p);
-if (av_parse_video_rate(frame_rate, arg) 0) {
-ERROR(Incorrect frame rate: %s\n, arg);
-} else {
-if (av_dict_set_int(config-video_conf, VideoFrameRateNum,
frame_rate.num, 0) 0 ||
-av_dict_set_int(config-video_conf, VideoFrameRateDen,
frame_rate.den, 0) 0)
+else {
+if ((w % 2) || (h % 2))
Drop the redundant () across %. Also, please make an effort
to break lines at 80 chars as long as it doesn't make the
code harder to read. This seems particularly possible on the
function declarations.
Other than these two minor nits, the patch seems OK
to push.
Thanks a lot.
--
Reynaldo H. Verdejo Pinochet
Open Source Group
Samsung Research America / Silicon Valley
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 03/11] ffserver_config: map ffserver options to AVOptions
Hi On 11/16/2014 11:03 PM, Lukasz Marek wrote: On 17.11.2014 02:46, Lukasz Marek wrote: Signed-off-by: Lukasz Marek lukasz.m.lu...@gmail.com --- ffserver_config.c | 237 -- ffserver_config.h | 2 - 2 files changed, 69 insertions(+), 170 deletions(-) @Reynaldo, you may check the test I submitted, and the comments, you may then have just high level review, not bother if mapping between options is ok Will do. Thanks for the hint. I should be able to take a look at this and the rest of the changes tomorrow. Bests, -- Reynaldo H. Verdejo Pinochet Open Source Group Samsung Research America / Silicon Valley ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 03/11] ffserver_config: map ffserver options to AVOptions
On 17.11.2014 02:46, Lukasz Marek wrote: Signed-off-by: Lukasz Marek lukasz.m.lu...@gmail.com --- ffserver_config.c | 237 -- ffserver_config.h | 2 - 2 files changed, 69 insertions(+), 170 deletions(-) @Reynaldo, you may check the test I submitted, and the comments, you may then have just high level review, not bother if mapping between options is ok ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
