diff mbox

[FFmpeg-devel,2/9] ffmpeg: do packet ts rescaling in write_packet()

Message ID 20170210123541.32375-3-nfxjfg@googlemail.com
State Superseded
Headers show

Commit Message

wm4 Feb. 10, 2017, 12:35 p.m. UTC 
From: Anton Khirnov <anton@khirnov.net>

This will be useful in the following commit, after which the muxer
timebase is not always available when encoding.

This merges Libav commit 3e265ca. It was previously skipped.

There is a minor change with setting the mux_timebase field only after
the muxer's write_header function has been called, because it can
readjust the timebase.

Includes a minor merge fix by Mark Thompson.

Signed-off-by: wm4 <nfxjfg@googlemail.com>
---
 ffmpeg.c | 39 ++++++++++++++++++++++-----------------
 ffmpeg.h |  2 ++
 2 files changed, 24 insertions(+), 17 deletions(-)

Comments

Michael Niedermayer Feb. 10, 2017, 9:43 p.m. UTC | #1 
On Fri, Feb 10, 2017 at 01:35:34PM +0100, wm4 wrote:
> From: Anton Khirnov <anton@khirnov.net>
> 
> This will be useful in the following commit, after which the muxer
> timebase is not always available when encoding.
> 
> This merges Libav commit 3e265ca. It was previously skipped.
> 
> There is a minor change with setting the mux_timebase field only after
> the muxer's write_header function has been called, because it can
> readjust the timebase.
> 
> Includes a minor merge fix by Mark Thompson.
> 
> Signed-off-by: wm4 <nfxjfg@googlemail.com>
> ---
>  ffmpeg.c | 39 ++++++++++++++++++++++-----------------
>  ffmpeg.h |  2 ++
>  2 files changed, 24 insertions(+), 17 deletions(-)

Is this intended to result in no changes ?

./ffmpeg -ss 15 -i ~/tickets/1332/Starship_Troopers.vob  -scodec dvbsub  -vcodec copy -t 4 -r 24000/1001 -an aaa.ts
differs from before and after this patch

[...]
wm4 Feb. 13, 2017, 5:27 a.m. UTC | #2 
On Fri, 10 Feb 2017 22:43:26 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Fri, Feb 10, 2017 at 01:35:34PM +0100, wm4 wrote:
> > From: Anton Khirnov <anton@khirnov.net>
> > 
> > This will be useful in the following commit, after which the muxer
> > timebase is not always available when encoding.
> > 
> > This merges Libav commit 3e265ca. It was previously skipped.
> > 
> > There is a minor change with setting the mux_timebase field only after
> > the muxer's write_header function has been called, because it can
> > readjust the timebase.
> > 
> > Includes a minor merge fix by Mark Thompson.
> > 
> > Signed-off-by: wm4 <nfxjfg@googlemail.com>
> > ---
> >  ffmpeg.c | 39 ++++++++++++++++++++++-----------------
> >  ffmpeg.h |  2 ++
> >  2 files changed, 24 insertions(+), 17 deletions(-)  
> 
> Is this intended to result in no changes ?
> 
> ./ffmpeg -ss 15 -i ~/tickets/1332/Starship_Troopers.vob  -scodec dvbsub  -vcodec copy -t 4 -r 24000/1001 -an aaa.ts
> differs from before and after this patch
> 
> [...]

Is it different in a good or a bad way?
Michael Niedermayer Feb. 14, 2017, 1:08 p.m. UTC | #3 
On Mon, Feb 13, 2017 at 06:27:50AM +0100, wm4 wrote:
> On Fri, 10 Feb 2017 22:43:26 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Fri, Feb 10, 2017 at 01:35:34PM +0100, wm4 wrote:
> > > From: Anton Khirnov <anton@khirnov.net>
> > > 
> > > This will be useful in the following commit, after which the muxer
> > > timebase is not always available when encoding.
> > > 
> > > This merges Libav commit 3e265ca. It was previously skipped.
> > > 
> > > There is a minor change with setting the mux_timebase field only after
> > > the muxer's write_header function has been called, because it can
> > > readjust the timebase.
> > > 
> > > Includes a minor merge fix by Mark Thompson.
> > > 
> > > Signed-off-by: wm4 <nfxjfg@googlemail.com>
> > > ---
> > >  ffmpeg.c | 39 ++++++++++++++++++++++-----------------
> > >  ffmpeg.h |  2 ++
> > >  2 files changed, 24 insertions(+), 17 deletions(-)  
> > 
> > Is this intended to result in no changes ?
> > 
> > ./ffmpeg -ss 15 -i ~/tickets/1332/Starship_Troopers.vob  -scodec dvbsub  -vcodec copy -t 4 -r 24000/1001 -an aaa.ts
> > differs from before and after this patch
> > 
> > [...]
> 
> Is it different in a good or a bad way?

I did not do a analysis of the difference.
My concern are the growing number of regressions (which implies more
get added than fixed). And so i try to find changes and issues in
patches before they are pushed.

I wish to work on the many past regressions and fix at least some
of them, i just realized we have some regressions that are caused by
commits i did long ago. These are things i want to look into and i
seem not to have as much time as i want. Analysing differences other
peoples patches on the ML cause is not something i would be able to do
without not doing something else that i feel iam more responsble for
Carl Eugen Hoyos Feb. 15, 2017, 10:48 a.m. UTC | #4 
2017-02-14 14:08 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
> My concern are the growing number of regressions

> (which implies more get added than fixed).

From a purely quantitative pov this is correct.

Carl Eugen
wm4 Feb. 15, 2017, 11 a.m. UTC | #5 
On Wed, 15 Feb 2017 11:48:36 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-02-14 14:08 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
> > My concern are the growing number of regressions  
> 
> > (which implies more get added than fixed).  
> 
> From a purely quantitative pov this is correct.

Do you propose we should reject all the hardware transcoding work?
Carl Eugen Hoyos Feb. 15, 2017, 11:06 a.m. UTC | #6 
2017-02-15 12:00 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Wed, 15 Feb 2017 11:48:36 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-02-14 14:08 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
>> > My concern are the growing number of regressions
>>
>> > (which implies more get added than fixed).
>>
>> From a purely quantitative pov this is correct.
>
> Do you propose we should reject all the hardware transcoding work?

I didn't propose anything, I just had the feeling Michael was writing
about a suggestion, so I wanted to clarify that he is correct.
Although now that you ask, I propose trying hard to fix a few
regressions before adding new ones.

Carl Eugen
diff mbox

Patch

diff --git a/ffmpeg.c b/ffmpeg.c
index 913c18d92b..8b829beb34 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -713,7 +713,7 @@  static void write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
             if (pkt->duration > 0)
                 av_log(NULL, AV_LOG_WARNING, "Overriding packet duration by frame rate, this should not happen\n");
             pkt->duration = av_rescale_q(1, av_inv_q(ost->frame_rate),
-                                         ost->st->time_base);
+                                         ost->mux_timebase);
         }
     }
 
@@ -759,6 +759,8 @@  static void write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
 
     pkt->stream_index = ost->index;
 
+    av_packet_rescale_ts(pkt, ost->mux_timebase, ost->st->time_base);
+
     if (debug_ts) {
         av_log(NULL, AV_LOG_INFO, "muxer <- type:%s "
                 "pkt_pts:%s pkt_pts_time:%s pkt_dts:%s pkt_dts_time:%s size:%d\n",
@@ -907,13 +909,13 @@  static void do_audio_out(OutputFile *of, OutputStream *ost,
 
         update_benchmark("encode_audio %d.%d", ost->file_index, ost->index);
 
-        av_packet_rescale_ts(&pkt, enc->time_base, ost->st->time_base);
+        av_packet_rescale_ts(&pkt, enc->time_base, ost->mux_timebase);
 
         if (debug_ts) {
             av_log(NULL, AV_LOG_INFO, "encoder -> type:audio "
                    "pkt_pts:%s pkt_pts_time:%s pkt_dts:%s pkt_dts_time:%s\n",
-                   av_ts2str(pkt.pts), av_ts2timestr(pkt.pts, &ost->st->time_base),
-                   av_ts2str(pkt.dts), av_ts2timestr(pkt.dts, &ost->st->time_base));
+                   av_ts2str(pkt.pts), av_ts2timestr(pkt.pts, &enc->time_base),
+                   av_ts2str(pkt.dts), av_ts2timestr(pkt.dts, &enc->time_base));
         }
 
         output_packet(of, &pkt, ost);
@@ -993,8 +995,8 @@  static void do_subtitle_out(OutputFile *of,
         av_init_packet(&pkt);
         pkt.data = subtitle_out;
         pkt.size = subtitle_out_size;
-        pkt.pts  = av_rescale_q(sub->pts, AV_TIME_BASE_Q, ost->st->time_base);
-        pkt.duration = av_rescale_q(sub->end_display_time, (AVRational){ 1, 1000 }, ost->st->time_base);
+        pkt.pts  = av_rescale_q(sub->pts, AV_TIME_BASE_Q, ost->mux_timebase);
+        pkt.duration = av_rescale_q(sub->end_display_time, (AVRational){ 1, 1000 }, ost->mux_timebase);
         if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE) {
             /* XXX: the pts correction is handled here. Maybe handling
                it in the codec would be better */
@@ -1187,7 +1189,7 @@  static void do_video_out(OutputFile *of,
             mux_par->field_order = AV_FIELD_PROGRESSIVE;
         pkt.data   = (uint8_t *)in_picture;
         pkt.size   =  sizeof(AVPicture);
-        pkt.pts    = av_rescale_q(in_picture->pts, enc->time_base, ost->st->time_base);
+        pkt.pts    = av_rescale_q(in_picture->pts, enc->time_base, ost->mux_timebase);
         pkt.flags |= AV_PKT_FLAG_KEY;
 
         output_packet(of, &pkt, ost);
@@ -1283,13 +1285,13 @@  static void do_video_out(OutputFile *of,
             if (pkt.pts == AV_NOPTS_VALUE && !(enc->codec->capabilities & AV_CODEC_CAP_DELAY))
                 pkt.pts = ost->sync_opts;
 
-            av_packet_rescale_ts(&pkt, enc->time_base, ost->st->time_base);
+            av_packet_rescale_ts(&pkt, enc->time_base, ost->mux_timebase);
 
             if (debug_ts) {
                 av_log(NULL, AV_LOG_INFO, "encoder -> type:video "
                     "pkt_pts:%s pkt_pts_time:%s pkt_dts:%s pkt_dts_time:%s\n",
-                    av_ts2str(pkt.pts), av_ts2timestr(pkt.pts, &ost->st->time_base),
-                    av_ts2str(pkt.dts), av_ts2timestr(pkt.dts, &ost->st->time_base));
+                    av_ts2str(pkt.pts), av_ts2timestr(pkt.pts, &ost->mux_timebase),
+                    av_ts2str(pkt.dts), av_ts2timestr(pkt.dts, &ost->mux_timebase));
             }
 
             frame_size = pkt.size;
@@ -1862,8 +1864,8 @@  static void flush_encoders(void)
                     av_packet_unref(&pkt);
                     continue;
                 }
-                av_packet_rescale_ts(&pkt, enc->time_base, ost->st->time_base);
                 pkt_size = pkt.size;
+                av_packet_rescale_ts(&pkt, enc->time_base, ost->mux_timebase);
                 output_packet(of, &pkt, ost);
                 if (ost->enc_ctx->codec_type == AVMEDIA_TYPE_VIDEO && vstats_filename) {
                     do_video_stats(ost, pkt_size);
@@ -1897,7 +1899,7 @@  static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
     OutputFile *of = output_files[ost->file_index];
     InputFile   *f = input_files [ist->file_index];
     int64_t start_time = (of->start_time == AV_NOPTS_VALUE) ? 0 : of->start_time;
-    int64_t ost_tb_start_time = av_rescale_q(start_time, AV_TIME_BASE_Q, ost->st->time_base);
+    int64_t ost_tb_start_time = av_rescale_q(start_time, AV_TIME_BASE_Q, ost->mux_timebase);
     AVPicture pict;
     AVPacket opkt;
 
@@ -1938,14 +1940,14 @@  static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
         ost->sync_opts++;
 
     if (pkt->pts != AV_NOPTS_VALUE)
-        opkt.pts = av_rescale_q(pkt->pts, ist->st->time_base, ost->st->time_base) - ost_tb_start_time;
+        opkt.pts = av_rescale_q(pkt->pts, ist->st->time_base, ost->mux_timebase) - ost_tb_start_time;
     else
         opkt.pts = AV_NOPTS_VALUE;
 
     if (pkt->dts == AV_NOPTS_VALUE)
-        opkt.dts = av_rescale_q(ist->dts, AV_TIME_BASE_Q, ost->st->time_base);
+        opkt.dts = av_rescale_q(ist->dts, AV_TIME_BASE_Q, ost->mux_timebase);
     else
-        opkt.dts = av_rescale_q(pkt->dts, ist->st->time_base, ost->st->time_base);
+        opkt.dts = av_rescale_q(pkt->dts, ist->st->time_base, ost->mux_timebase);
     opkt.dts -= ost_tb_start_time;
 
     if (ost->st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && pkt->dts != AV_NOPTS_VALUE) {
@@ -1954,10 +1956,11 @@  static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
             duration = ist->dec_ctx->frame_size;
         opkt.dts = opkt.pts = av_rescale_delta(ist->st->time_base, pkt->dts,
                                                (AVRational){1, ist->dec_ctx->sample_rate}, duration, &ist->filter_in_rescale_delta_last,
-                                               ost->st->time_base) - ost_tb_start_time;
+                                               ost->mux_timebase) - ost_tb_start_time;
     }
 
-    opkt.duration = av_rescale_q(pkt->duration, ist->st->time_base, ost->st->time_base);
+    opkt.duration = av_rescale_q(pkt->duration, ist->st->time_base, ost->mux_timebase);
+
     opkt.flags    = pkt->flags;
     // FIXME remove the following 2 lines they shall be replaced by the bitstream filters
     if (  ost->st->codecpar->codec_id != AV_CODEC_ID_H264
@@ -3410,6 +3413,8 @@  static int init_output_stream(OutputStream *ost, char *error, int error_len)
     if (ret < 0)
         return ret;
 
+    ost->mux_timebase = ost->st->time_base;
+
     return ret;
 }
 
diff --git a/ffmpeg.h b/ffmpeg.h
index 458bb8a3dc..ca35ccc260 100644
--- a/ffmpeg.h
+++ b/ffmpeg.h
@@ -446,6 +446,8 @@  typedef struct OutputStream {
     int64_t first_pts;
     /* dts of the last packet sent to the muxer */
     int64_t last_mux_dts;
+    // the timebase of the packets sent to the muxer
+    AVRational mux_timebase;
 
     int                    nb_bitstream_filters;
     uint8_t                  *bsf_extradata_updated;