On 10/31/2016 04:56 PM, Nicolas George wrote:
+ if (av_match_name(use_fifo, "true,y,yes,enable,enabled,on,1")) {
+ tee_slave->use_fifo = 1;
+ } else if (av_match_name(use_fifo,
"false,n,no,disable,disabled,off,0")) {
I am not happy about the duplication of the tests from
set_string_bool(), but it cannot be avoided for now without more
unrelated work.
(I really with each option type came with the corresponding silent and
verbose parsing and dump functions. Unfortunately, this discipline was
not kept in the past.)
It may be worth refactoring just these tests setting variable to 0/1/-1
based on boolean/"auto" string to separate function. If you agree I can
do this in next patch
+ ret = avformat_alloc_output_context2(&avf2, NULL, "fifo", filename);
+ } else {
+ ret = avformat_alloc_output_context2(&avf2, NULL, format, filename);
You could probably factor these two lines with
"use_fifo ? "fifo" : format".
Thanks, looks nicer that way, I've modified it.
+ }
if (ret < 0)
goto end;
tee_slave->avf = avf2;
@@ -394,6 +467,12 @@ static int tee_write_header(AVFormatContext *avf)
filename++;
}
+ if (tee->fifo_options_str) {
+ ret = av_dict_parse_string(&tee->fifo_options, tee->fifo_options_str, "=",
":", 0);
+ if (ret < 0)
+ goto fail;
+ }
+
if (!(tee->slaves = av_mallocz_array(nb_slaves, sizeof(*tee->slaves)))) {
ret = AVERROR(ENOMEM);
goto fail;
@@ -401,6 +480,12 @@ static int tee_write_header(AVFormatContext *avf)
tee->nb_slaves = tee->nb_alive = nb_slaves;
for (i = 0; i < nb_slaves; i++) {
+
+ tee->slaves[i].use_fifo = tee->use_fifo;
+ ret = av_dict_copy(&tee->slaves[i].fifo_options, tee->fifo_options, 0);
Unless I am mistaken, if there are keys present in both dicts, the
entries in tee->fifo_options will overwrite the ones in
slave[i].fifo_options: in other words, global overrides local. Usually,
people want it the other way around.
This is executed before open_slave() function is run for that slave. So
the options "inherited" from global tee->fifo_options will be
overwritten by per-slave options in open_slave(). I think this is OK.
+ if (ret < 0)
+ goto fail;
+
if ((ret = open_slave(avf, slaves[i], &tee->slaves[i])) < 0) {
ret = tee_process_slave_failure(avf, i, ret);
if (ret < 0)
In short, there are a few points that could be IMHO slightly better, but
I think the patch can go in as is if you want to move forward, possibly
with a few /* TODO */ (but no need to send the patch again if you add
them).
Hum. Now I realize I have some doubts about the way the options work.
But with the current state of the options parsing, I am not sure we can
do much better.
Maybe a small suggestion: instead of stealing the fifo_options option,
steal all options starting with "fifo." (av_dict_get() can do that).
That would avoid one level of escaping. But it can also be added later
if you prefer.
I like this idea and have implemented it, now I am wondering if I should
keep both possibilities of how to pass options to fifo muxer... What do
you think?
Also, I am sorry for such late response to your review...
Thank you,
Regards,
Jan S.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
