Re: [FFmpeg-devel] [PATCH v2 1/3] ffmpeg_opt: pass output framerate as a hint to the encoder

2017-03-02 Thread Michael Niedermayer 
On Thu, Mar 02, 2017 at 03:35:08PM +0100, Tobias Rapp wrote:
> On 02.03.2017 03:27, Michael Niedermayer wrote:
> >On Mon, Feb 27, 2017 at 09:50:31AM +0100, Tobias Rapp wrote:
> >>On 26.02.2017 16:09, Michael Niedermayer wrote:
> >>>On Mon, Feb 20, 2017 at 04:05:00PM +0100, Tobias Rapp wrote:
> On 20.02.2017 15:09, Mark Thompson wrote:
> >On 06/02/17 12:33, Tobias Rapp wrote:
> >>Sets framerate field in output codec context if explicitly specified on
> >>the command-line.
> >>
> >>Signed-off-by: Tobias Rapp 
> >>---
> >>ffmpeg_opt.c | 2 ++
> >>1 file changed, 2 insertions(+)
> >>
> >>diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> >>index 6a47d32..3b532da 100644
> >>--- a/ffmpeg_opt.c
> >>+++ b/ffmpeg_opt.c
> >>@@ -1535,6 +1535,8 @@ static OutputStream 
> >>*new_video_stream(OptionsContext *o, AVFormatContext *oc, in
> >>   av_log(NULL, AV_LOG_FATAL, "Invalid framerate value: %s\n", 
> >> frame_rate);
> >>   exit_program(1);
> >>   }
> >>+if (frame_rate && ost->frame_rate.num && ost->frame_rate.den)
> >>+video_enc->framerate = ost->frame_rate;
> >>   if (frame_rate && video_sync_method == VSYNC_PASSTHROUGH)
> >>   av_log(NULL, AV_LOG_ERROR, "Using -vsync 0 and -r can produce 
> >> invalid output files\n");
> >>
> >>
> >
> >Is there a reason not to always set this, rather than only when it is 
> >specified explicitly on the command line as you have?
> >
> >(Like 
> >,
> > though that is after the current merge point and I don't know if it 
> >interacts with anything else.)
> 
> I just tried to be extra cautious. Merging Libav commit
> d10102d23c9467d4eb84f58e0cd12be284b982f6 would provide a more
> general solution and might be preferred.
> >>>
> >>>that one would change fate results, are the changed results better or
> >>>worse ?
> >>
> >>I rebased unto current master and run fate with the attached change
> >>to ffmpeg.c applied but couldn't find any changed reference files.
> >>
> >
> >>Which fate tests had some changes and which platform did you check?
> >
> >there was a commit in origin/master that broke the tests, i realized
> >that after sending the mail but was too busy to reply immedeaty and
> >then forgot
> 
> No problem. So I consider my original patch 1/3 as dropped.
> 

> What is the best way to continue, assuming that the other two
> patches in the series are OK?

> 
> Shall I merge the single Libav commit d10102d2 followed by the other
> two patches? Or shall I wait until the general Libav merge-queue up
> to d10102d2 has been processed (~210 commits)? Or just wait for the
> merges affecting ffmpeg.c (~4 commits)?

waiting is generally bad for paralellism
if you need a patch, cherry pick it


[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2 1/3] ffmpeg_opt: pass output framerate as a hint to the encoder

2017-03-02 Thread Tobias Rapp 

On 02.03.2017 03:27, Michael Niedermayer wrote:

On Mon, Feb 27, 2017 at 09:50:31AM +0100, Tobias Rapp wrote:

On 26.02.2017 16:09, Michael Niedermayer wrote:

On Mon, Feb 20, 2017 at 04:05:00PM +0100, Tobias Rapp wrote:

On 20.02.2017 15:09, Mark Thompson wrote:

On 06/02/17 12:33, Tobias Rapp wrote:

Sets framerate field in output codec context if explicitly specified on
the command-line.

Signed-off-by: Tobias Rapp 
---
ffmpeg_opt.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index 6a47d32..3b532da 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -1535,6 +1535,8 @@ static OutputStream *new_video_stream(OptionsContext *o, 
AVFormatContext *oc, in
   av_log(NULL, AV_LOG_FATAL, "Invalid framerate value: %s\n", frame_rate);
   exit_program(1);
   }
+if (frame_rate && ost->frame_rate.num && ost->frame_rate.den)
+video_enc->framerate = ost->frame_rate;
   if (frame_rate && video_sync_method == VSYNC_PASSTHROUGH)
   av_log(NULL, AV_LOG_ERROR, "Using -vsync 0 and -r can produce invalid output 
files\n");




Is there a reason not to always set this, rather than only when it is specified 
explicitly on the command line as you have?

(Like 
,
 though that is after the current merge point and I don't know if it interacts with 
anything else.)


I just tried to be extra cautious. Merging Libav commit
d10102d23c9467d4eb84f58e0cd12be284b982f6 would provide a more
general solution and might be preferred.


that one would change fate results, are the changed results better or
worse ?


I rebased unto current master and run fate with the attached change
to ffmpeg.c applied but couldn't find any changed reference files.




Which fate tests had some changes and which platform did you check?


there was a commit in origin/master that broke the tests, i realized
that after sending the mail but was too busy to reply immedeaty and
then forgot


No problem. So I consider my original patch 1/3 as dropped.

What is the best way to continue, assuming that the other two patches in 
the series are OK?


Shall I merge the single Libav commit d10102d2 followed by the other two 
patches? Or shall I wait until the general Libav merge-queue up to 
d10102d2 has been processed (~210 commits)? Or just wait for the merges 
affecting ffmpeg.c (~4 commits)?


Another option would be to just apply the other two patches as they are 
passing FATE without this one (framerate is guessed from stream timebase 
then).


Regards,
Tobias

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

Re: [FFmpeg-devel] [PATCH v2 1/3] ffmpeg_opt: pass output framerate as a hint to the encoder

2017-03-01 Thread Michael Niedermayer 
On Mon, Feb 27, 2017 at 09:50:31AM +0100, Tobias Rapp wrote:
> On 26.02.2017 16:09, Michael Niedermayer wrote:
> >On Mon, Feb 20, 2017 at 04:05:00PM +0100, Tobias Rapp wrote:
> >>On 20.02.2017 15:09, Mark Thompson wrote:
> >>>On 06/02/17 12:33, Tobias Rapp wrote:
> Sets framerate field in output codec context if explicitly specified on
> the command-line.
> 
> Signed-off-by: Tobias Rapp 
> ---
> ffmpeg_opt.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> index 6a47d32..3b532da 100644
> --- a/ffmpeg_opt.c
> +++ b/ffmpeg_opt.c
> @@ -1535,6 +1535,8 @@ static OutputStream 
> *new_video_stream(OptionsContext *o, AVFormatContext *oc, in
> av_log(NULL, AV_LOG_FATAL, "Invalid framerate value: %s\n", 
>  frame_rate);
> exit_program(1);
> }
> +if (frame_rate && ost->frame_rate.num && ost->frame_rate.den)
> +video_enc->framerate = ost->frame_rate;
> if (frame_rate && video_sync_method == VSYNC_PASSTHROUGH)
> av_log(NULL, AV_LOG_ERROR, "Using -vsync 0 and -r can produce 
>  invalid output files\n");
> 
> 
> >>>
> >>>Is there a reason not to always set this, rather than only when it is 
> >>>specified explicitly on the command line as you have?
> >>>
> >>>(Like 
> >>>,
> >>> though that is after the current merge point and I don't know if it 
> >>>interacts with anything else.)
> >>
> >>I just tried to be extra cautious. Merging Libav commit
> >>d10102d23c9467d4eb84f58e0cd12be284b982f6 would provide a more
> >>general solution and might be preferred.
> >
> >that one would change fate results, are the changed results better or
> >worse ?
> 
> I rebased unto current master and run fate with the attached change
> to ffmpeg.c applied but couldn't find any changed reference files.
> 

> Which fate tests had some changes and which platform did you check?

there was a commit in origin/master that broke the tests, i realized
that after sending the mail but was too busy to reply immedeaty and
then forgot

[...]


-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2 1/3] ffmpeg_opt: pass output framerate as a hint to the encoder

2017-02-27 Thread Tobias Rapp 

On 26.02.2017 16:09, Michael Niedermayer wrote:

On Mon, Feb 20, 2017 at 04:05:00PM +0100, Tobias Rapp wrote:

On 20.02.2017 15:09, Mark Thompson wrote:

On 06/02/17 12:33, Tobias Rapp wrote:

Sets framerate field in output codec context if explicitly specified on
the command-line.

Signed-off-by: Tobias Rapp 
---
ffmpeg_opt.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index 6a47d32..3b532da 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -1535,6 +1535,8 @@ static OutputStream *new_video_stream(OptionsContext *o, 
AVFormatContext *oc, in
av_log(NULL, AV_LOG_FATAL, "Invalid framerate value: %s\n", frame_rate);
exit_program(1);
}
+if (frame_rate && ost->frame_rate.num && ost->frame_rate.den)
+video_enc->framerate = ost->frame_rate;
if (frame_rate && video_sync_method == VSYNC_PASSTHROUGH)
av_log(NULL, AV_LOG_ERROR, "Using -vsync 0 and -r can produce invalid output 
files\n");




Is there a reason not to always set this, rather than only when it is specified 
explicitly on the command line as you have?

(Like 
,
 though that is after the current merge point and I don't know if it interacts with 
anything else.)


I just tried to be extra cautious. Merging Libav commit
d10102d23c9467d4eb84f58e0cd12be284b982f6 would provide a more
general solution and might be preferred.


that one would change fate results, are the changed results better or
worse ?


I rebased unto current master and run fate with the attached change to 
ffmpeg.c applied but couldn't find any changed reference files.


Which fate tests had some changes and which platform did you check?

Regards,
Tobias
diff --git a/ffmpeg.c b/ffmpeg.c
index 06570c0..333a806 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -3202,6 +3202,8 @@ static int init_output_stream_encode(OutputStream *ost)
 enc_ctx->bits_per_raw_sample = FFMIN(dec_ctx->bits_per_raw_sample,
  
av_pix_fmt_desc_get(enc_ctx->pix_fmt)->comp[0].depth);
 
+enc_ctx->framerate = ost->frame_rate;
+
 ost->st->avg_frame_rate = ost->frame_rate;
 
 if (!dec_ctx ||
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2 1/3] ffmpeg_opt: pass output framerate as a hint to the encoder

2017-02-26 Thread Michael Niedermayer 
On Mon, Feb 20, 2017 at 04:05:00PM +0100, Tobias Rapp wrote:
> On 20.02.2017 15:09, Mark Thompson wrote:
> >On 06/02/17 12:33, Tobias Rapp wrote:
> >>Sets framerate field in output codec context if explicitly specified on
> >>the command-line.
> >>
> >>Signed-off-by: Tobias Rapp 
> >>---
> >> ffmpeg_opt.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >>diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> >>index 6a47d32..3b532da 100644
> >>--- a/ffmpeg_opt.c
> >>+++ b/ffmpeg_opt.c
> >>@@ -1535,6 +1535,8 @@ static OutputStream *new_video_stream(OptionsContext 
> >>*o, AVFormatContext *oc, in
> >> av_log(NULL, AV_LOG_FATAL, "Invalid framerate value: %s\n", 
> >> frame_rate);
> >> exit_program(1);
> >> }
> >>+if (frame_rate && ost->frame_rate.num && ost->frame_rate.den)
> >>+video_enc->framerate = ost->frame_rate;
> >> if (frame_rate && video_sync_method == VSYNC_PASSTHROUGH)
> >> av_log(NULL, AV_LOG_ERROR, "Using -vsync 0 and -r can produce 
> >> invalid output files\n");
> >>
> >>
> >
> >Is there a reason not to always set this, rather than only when it is 
> >specified explicitly on the command line as you have?
> >
> >(Like 
> >,
> > though that is after the current merge point and I don't know if it 
> >interacts with anything else.)
> 
> I just tried to be extra cautious. Merging Libav commit
> d10102d23c9467d4eb84f58e0cd12be284b982f6 would provide a more
> general solution and might be preferred.

that one would change fate results, are the changed results better or
worse ?

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2 1/3] ffmpeg_opt: pass output framerate as a hint to the encoder

2017-02-20 Thread Tobias Rapp 

On 20.02.2017 15:09, Mark Thompson wrote:

On 06/02/17 12:33, Tobias Rapp wrote:

Sets framerate field in output codec context if explicitly specified on
the command-line.

Signed-off-by: Tobias Rapp 
---
 ffmpeg_opt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index 6a47d32..3b532da 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -1535,6 +1535,8 @@ static OutputStream *new_video_stream(OptionsContext *o, 
AVFormatContext *oc, in
 av_log(NULL, AV_LOG_FATAL, "Invalid framerate value: %s\n", 
frame_rate);
 exit_program(1);
 }
+if (frame_rate && ost->frame_rate.num && ost->frame_rate.den)
+video_enc->framerate = ost->frame_rate;
 if (frame_rate && video_sync_method == VSYNC_PASSTHROUGH)
 av_log(NULL, AV_LOG_ERROR, "Using -vsync 0 and -r can produce invalid 
output files\n");




Is there a reason not to always set this, rather than only when it is specified 
explicitly on the command line as you have?

(Like 
,
 though that is after the current merge point and I don't know if it interacts with 
anything else.)


I just tried to be extra cautious. Merging Libav commit 
d10102d23c9467d4eb84f58e0cd12be284b982f6 would provide a more general 
solution and might be preferred.


Regards,
Tobias

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

Re: [FFmpeg-devel] [PATCH v2 1/3] ffmpeg_opt: pass output framerate as a hint to the encoder

2017-02-20 Thread Mark Thompson 
On 06/02/17 12:33, Tobias Rapp wrote:
> Sets framerate field in output codec context if explicitly specified on
> the command-line.
> 
> Signed-off-by: Tobias Rapp 
> ---
>  ffmpeg_opt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> index 6a47d32..3b532da 100644
> --- a/ffmpeg_opt.c
> +++ b/ffmpeg_opt.c
> @@ -1535,6 +1535,8 @@ static OutputStream *new_video_stream(OptionsContext 
> *o, AVFormatContext *oc, in
>  av_log(NULL, AV_LOG_FATAL, "Invalid framerate value: %s\n", 
> frame_rate);
>  exit_program(1);
>  }
> +if (frame_rate && ost->frame_rate.num && ost->frame_rate.den)
> +video_enc->framerate = ost->frame_rate;
>  if (frame_rate && video_sync_method == VSYNC_PASSTHROUGH)
>  av_log(NULL, AV_LOG_ERROR, "Using -vsync 0 and -r can produce 
> invalid output files\n");
>  
> 

Is there a reason not to always set this, rather than only when it is specified 
explicitly on the command line as you have?

(Like 
,
 though that is after the current merge point and I don't know if it interacts 
with anything else.)

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

Re: [FFmpeg-devel] [PATCH v2 1/3] ffmpeg_opt: pass output framerate as a hint to the encoder

2017-02-20 Thread Tobias Rapp 

On 13.02.2017 08:51, Tobias Rapp wrote:

On 06.02.2017 13:33, Tobias Rapp wrote:

Sets framerate field in output codec context if explicitly specified on
the command-line.

Signed-off-by: Tobias Rapp 
---
 ffmpeg_opt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index 6a47d32..3b532da 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -1535,6 +1535,8 @@ static OutputStream
*new_video_stream(OptionsContext *o, AVFormatContext *oc, in
 av_log(NULL, AV_LOG_FATAL, "Invalid framerate value: %s\n",
frame_rate);
 exit_program(1);
 }
+if (frame_rate && ost->frame_rate.num && ost->frame_rate.den)
+video_enc->framerate = ost->frame_rate;
 if (frame_rate && video_sync_method == VSYNC_PASSTHROUGH)
 av_log(NULL, AV_LOG_ERROR, "Using -vsync 0 and -r can produce
invalid output files\n");



Ping on the patch series.


Ping. Having better bitrate estimation improves automatic AVI index 
space reservation (commit e65db4ce5966506d957032ef30545419801ae7dc) for 
non-constant bitrate use-cases.


Regards,
Tobias

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

Re: [FFmpeg-devel] [PATCH v2 1/3] ffmpeg_opt: pass output framerate as a hint to the encoder

2017-02-12 Thread Tobias Rapp 

On 06.02.2017 13:33, Tobias Rapp wrote:

Sets framerate field in output codec context if explicitly specified on
the command-line.

Signed-off-by: Tobias Rapp 
---
 ffmpeg_opt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index 6a47d32..3b532da 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -1535,6 +1535,8 @@ static OutputStream *new_video_stream(OptionsContext *o, 
AVFormatContext *oc, in
 av_log(NULL, AV_LOG_FATAL, "Invalid framerate value: %s\n", 
frame_rate);
 exit_program(1);
 }
+if (frame_rate && ost->frame_rate.num && ost->frame_rate.den)
+video_enc->framerate = ost->frame_rate;
 if (frame_rate && video_sync_method == VSYNC_PASSTHROUGH)
 av_log(NULL, AV_LOG_ERROR, "Using -vsync 0 and -r can produce invalid 
output files\n");



Ping on the patch series.

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

[FFmpeg-devel] [PATCH v2 1/3] ffmpeg_opt: pass output framerate as a hint to the encoder

2017-02-06 Thread Tobias Rapp 
Sets framerate field in output codec context if explicitly specified on
the command-line.

Signed-off-by: Tobias Rapp 
---
 ffmpeg_opt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index 6a47d32..3b532da 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -1535,6 +1535,8 @@ static OutputStream *new_video_stream(OptionsContext *o, 
AVFormatContext *oc, in
 av_log(NULL, AV_LOG_FATAL, "Invalid framerate value: %s\n", 
frame_rate);
 exit_program(1);
 }
+if (frame_rate && ost->frame_rate.num && ost->frame_rate.den)
+video_enc->framerate = ost->frame_rate;
 if (frame_rate && video_sync_method == VSYNC_PASSTHROUGH)
 av_log(NULL, AV_LOG_ERROR, "Using -vsync 0 and -r can produce invalid 
output files\n");
 
-- 
2.7.4


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