[FFmpeg-devel,1/2] fftools/ffmpeg: Refine the do_video_out.

Submitted by Jun Zhao on Nov. 8, 2018, 3:35 p.m.

Details

Message ID 1541691321-12543-1-git-send-email-mypopydev@gmail.com
State New
Headers show

Commit Message

Jun Zhao Nov. 8, 2018, 3:35 p.m.
Signed-off-by: Jun Zhao <mypopydev@gmail.com>
---
 fftools/ffmpeg.c |   62 ++++++++++++++++++++++++-----------------------------
 1 files changed, 28 insertions(+), 34 deletions(-)

Comments

Carl Eugen Hoyos Nov. 8, 2018, 3:45 p.m.
2018-11-08 16:35 GMT+01:00, Jun Zhao <mypopydev@gmail.com>:
> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> ---
>  fftools/ffmpeg.c |   62
> ++++++++++++++++++++++++-----------------------------
>  1 files changed, 28 insertions(+), 34 deletions(-)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index da4259a..e989e7a 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -1193,33 +1193,27 @@ static void do_video_out(OutputFile *of,
>      }
>      ost->last_dropped = nb_frames == nb0_frames && next_picture;
>
> -  /* duplicates frame if needed */
> -  for (i = 0; i < nb_frames; i++) {
> -    AVFrame *in_picture;
> -    av_init_packet(&pkt);
> -    pkt.data = NULL;
> -    pkt.size = 0;
> -
> -    if (i < nb0_frames && ost->last_frame) {
> -        in_picture = ost->last_frame;
> -    } else
> -        in_picture = next_picture;
> +    /* duplicates frame if needed */
> +    for (i = 0; i < nb_frames; i++) {
> +        AVFrame *in_picture;
> +        int forced_keyframe = 0;
> +        double pts_time;
> +        av_init_packet(&pkt);
> +        pkt.data = NULL;
> +        pkt.size = 0;
>
> -    if (!in_picture)
> -        return;
> +        if (i < nb0_frames && ost->last_frame) {
> +            in_picture = ost->last_frame;
> +        } else
> +            in_picture = next_picture;

This patch seems to mix functional changes with re-indentation.
The patch will be much easier to review if you move all re-indentation
into the second patch.

Carl Eugen
Michael Niedermayer Nov. 8, 2018, 8:24 p.m.
On Thu, Nov 08, 2018 at 11:35:20PM +0800, Jun Zhao wrote:
> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> ---
>  fftools/ffmpeg.c |   62 ++++++++++++++++++++++++-----------------------------
>  1 files changed, 28 insertions(+), 34 deletions(-)

The commit message "Refine the do_video_out." is too terse
its not clear from it what is done, why it is done, how it is done

thx

[...]
mypopy@gmail.com Nov. 9, 2018, 12:45 a.m.
On Thu, Nov 8, 2018 at 11:45 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> 2018-11-08 16:35 GMT+01:00, Jun Zhao <mypopydev@gmail.com>:
> > Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> > ---
> >  fftools/ffmpeg.c |   62
> > ++++++++++++++++++++++++-----------------------------
> >  1 files changed, 28 insertions(+), 34 deletions(-)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index da4259a..e989e7a 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -1193,33 +1193,27 @@ static void do_video_out(OutputFile *of,
> >      }
> >      ost->last_dropped = nb_frames == nb0_frames && next_picture;
> >
> > -  /* duplicates frame if needed */
> > -  for (i = 0; i < nb_frames; i++) {
> > -    AVFrame *in_picture;
> > -    av_init_packet(&pkt);
> > -    pkt.data = NULL;
> > -    pkt.size = 0;
> > -
> > -    if (i < nb0_frames && ost->last_frame) {
> > -        in_picture = ost->last_frame;
> > -    } else
> > -        in_picture = next_picture;
> > +    /* duplicates frame if needed */
> > +    for (i = 0; i < nb_frames; i++) {
> > +        AVFrame *in_picture;
> > +        int forced_keyframe = 0;
> > +        double pts_time;
> > +        av_init_packet(&pkt);
> > +        pkt.data = NULL;
> > +        pkt.size = 0;
> >
> > -    if (!in_picture)
> > -        return;
> > +        if (i < nb0_frames && ost->last_frame) {
> > +            in_picture = ost->last_frame;
> > +        } else
> > +            in_picture = next_picture;
>
> This patch seems to mix functional changes with re-indentation.
> The patch will be much easier to review if you move all re-indentation
> into the second patch.
>
> Carl Eugen
Will split the patch and move the re-indentataion to 2nd patch, Thanks
mypopy@gmail.com Nov. 9, 2018, 12:47 a.m.
On Fri, Nov 9, 2018 at 4:24 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Thu, Nov 08, 2018 at 11:35:20PM +0800, Jun Zhao wrote:
> > Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> > ---
> >  fftools/ffmpeg.c |   62 ++++++++++++++++++++++++-----------------------------
> >  1 files changed, 28 insertions(+), 34 deletions(-)
>
> The commit message "Refine the do_video_out." is too terse
> its not clear from it what is done, why it is done, how it is done
>
> thx
>
The v2 patchset will give more detail information in commit message,
thanks the review.

Patch hide | download patch | download mbox

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index da4259a..e989e7a 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -1193,33 +1193,27 @@  static void do_video_out(OutputFile *of,
     }
     ost->last_dropped = nb_frames == nb0_frames && next_picture;
 
-  /* duplicates frame if needed */
-  for (i = 0; i < nb_frames; i++) {
-    AVFrame *in_picture;
-    av_init_packet(&pkt);
-    pkt.data = NULL;
-    pkt.size = 0;
-
-    if (i < nb0_frames && ost->last_frame) {
-        in_picture = ost->last_frame;
-    } else
-        in_picture = next_picture;
+    /* duplicates frame if needed */
+    for (i = 0; i < nb_frames; i++) {
+        AVFrame *in_picture;
+        int forced_keyframe = 0;
+        double pts_time;
+        av_init_packet(&pkt);
+        pkt.data = NULL;
+        pkt.size = 0;
 
-    if (!in_picture)
-        return;
+        if (i < nb0_frames && ost->last_frame) {
+            in_picture = ost->last_frame;
+        } else
+            in_picture = next_picture;
 
-    in_picture->pts = ost->sync_opts;
+        if (!in_picture)
+            return;
 
-#if 1
-    if (!check_recording_time(ost))
-#else
-    if (ost->frame_number >= ost->max_frames)
-#endif
-        return;
+        in_picture->pts = ost->sync_opts;
 
-    {
-        int forced_keyframe = 0;
-        double pts_time;
+        if (!check_recording_time(ost))
+            return;
 
         if (enc->flags & (AV_CODEC_FLAG_INTERLACED_DCT | AV_CODEC_FLAG_INTERLACED_ME) &&
             ost->top_field_first >= 0)
@@ -1328,18 +1322,18 @@  static void do_video_out(OutputFile *of,
                 fprintf(ost->logfile, "%s", enc->stats_out);
             }
         }
-    }
-    ost->sync_opts++;
-    /*
-     * For video, number of frames in == number of packets out.
-     * But there may be reordering, so we can't throw away frames on encoder
-     * flush, we need to limit them here, before they go into encoder.
-     */
-    ost->frame_number++;
 
-    if (vstats_filename && frame_size)
-        do_video_stats(ost, frame_size);
-  }
+        ost->sync_opts++;
+        /*
+         * For video, number of frames in == number of packets out.
+         * But there may be reordering, so we can't throw away frames on encoder
+         * flush, we need to limit them here, before they go into encoder.
+         */
+        ost->frame_number++;
+
+        if (vstats_filename && frame_size)
+            do_video_stats(ost, frame_size);
+    }
 
     if (!ost->last_frame)
         ost->last_frame = av_frame_alloc();