| Message ID | 20170603011722.7896-1-isasi@google.com |
|---|---|
| State | Accepted |
| Commit | 93db5e3fc41ac0242acab86c3e4ce3a3dfb80075 |
| Headers | show |
On Fri, 2 Jun 2017 18:17:22 -0700 Sasi Inguva <isasi-at-google.com@ffmpeg.org> wrote: > Fixes t/6421 > > Signed-off-by: Sasi Inguva <isasi@google.com> > --- > libavformat/mov.c | 57 ++++++++++++++++++++++++---------- > tests/ref/fate/h264-twofields-packet | 60 ++++++++++++++++++------------------ > 2 files changed, 70 insertions(+), 47 deletions(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index 3845e63b53..d7d64c3361 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -3039,6 +3039,9 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) > int64_t edit_list_dts_entry_end = 0; > int64_t edit_list_start_ctts_sample = 0; > int64_t curr_cts; > + int64_t curr_ctts = 0; > + int64_t min_corrected_pts = -1; > + int64_t empty_edits_sum_duration = 0; > int64_t edit_list_index = 0; > int64_t index; > int64_t index_ctts_count; > @@ -3053,6 +3056,7 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) > int first_non_zero_audio_edit = -1; > int packet_skip_samples = 0; > MOVIndexRange *current_index_range; > + int i; > > if (!msc->elst_data || msc->elst_count <= 0 || nb_old <= 0) { > return; > @@ -3080,13 +3084,9 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) > > // If the dts_shift is positive (in case of negative ctts values in mov), > // then negate the DTS by dts_shift > - if (msc->dts_shift > 0) > + if (msc->dts_shift > 0) { > edit_list_dts_entry_end -= msc->dts_shift; > - > - // Offset the DTS by ctts[0] to make the PTS of the first frame 0 > - if (ctts_data_old && ctts_count_old > 0) { > - edit_list_dts_entry_end -= ctts_data_old[0].duration; > - av_log(mov->fc, AV_LOG_DEBUG, "Offset DTS by ctts[%d].duration: %d\n", 0, ctts_data_old[0].duration); > + av_log(mov->fc, AV_LOG_DEBUG, "Shifting DTS by %d because of negative CTTS.\n", msc->dts_shift); > } > > start_dts = edit_list_dts_entry_end; > @@ -3100,6 +3100,7 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) > edit_list_dts_entry_end += edit_list_duration; > num_discarded_begin = 0; > if (edit_list_media_time == -1) { > + empty_edits_sum_duration += edit_list_duration; > continue; > } > > @@ -3179,11 +3180,13 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) > > // frames (pts) before or after edit list > curr_cts = current->timestamp + msc->dts_shift; > + curr_ctts = 0; > > if (ctts_data_old && ctts_index_old < ctts_count_old) { > - av_log(mov->fc, AV_LOG_DEBUG, "shifted frame pts, curr_cts: %"PRId64" @ %"PRId64", ctts: %d, ctts_count: %"PRId64"\n", > - curr_cts, ctts_index_old, ctts_data_old[ctts_index_old].duration, ctts_count_old); > - curr_cts += ctts_data_old[ctts_index_old].duration; > + curr_ctts = ctts_data_old[ctts_index_old].duration; > + av_log(mov->fc, AV_LOG_DEBUG, "stts: %"PRId64" ctts: %"PRId64", ctts_index: %"PRId64", ctts_count: %"PRId64"\n", > + curr_cts, curr_ctts, ctts_index_old, ctts_count_old); > + curr_cts += curr_ctts; > ctts_sample_old++; > if (ctts_sample_old == ctts_data_old[ctts_index_old].count) { > if (add_ctts_entry(&msc->ctts_data, &msc->ctts_count, > @@ -3244,14 +3247,21 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) > } > } > } > - } else if (edit_list_start_encountered == 0) { > - edit_list_start_encountered = 1; > - // Make timestamps strictly monotonically increasing for audio, by rewriting timestamps for > - // discarded packets. > - if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && frame_duration_buffer) { > - fix_index_entry_timestamps(st, st->nb_index_entries, edit_list_dts_counter, > - frame_duration_buffer, num_discarded_begin); > - av_freep(&frame_duration_buffer); > + } else { > + if (min_corrected_pts < 0) { > + min_corrected_pts = edit_list_dts_counter + curr_ctts + msc->dts_shift; > + } else { > + min_corrected_pts = FFMIN(min_corrected_pts, edit_list_dts_counter + curr_ctts + msc->dts_shift); > + } > + if (edit_list_start_encountered == 0) { > + edit_list_start_encountered = 1; > + // Make timestamps strictly monotonically increasing for audio, by rewriting timestamps for > + // discarded packets. > + if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && frame_duration_buffer) { > + fix_index_entry_timestamps(st, st->nb_index_entries, edit_list_dts_counter, > + frame_duration_buffer, num_discarded_begin); > + av_freep(&frame_duration_buffer); > + } > } > } > > @@ -3292,6 +3302,19 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) > } > } > } > + // If there are empty edits, then min_corrected_pts might be positive intentionally. So we subtract the > + // sum duration of emtpy edits here. > + min_corrected_pts -= empty_edits_sum_duration; > + > + // If the minimum pts turns out to be greater than zero after fixing the index, then we subtract the > + // dts by that amount to make the first pts zero. > + if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && min_corrected_pts > 0) { > + av_log(mov->fc, AV_LOG_DEBUG, "Offset DTS by %"PRId64" to make first pts zero.\n", min_corrected_pts); > + for (i = 0; i < st->nb_index_entries; ++i) { > + st->index_entries[i].timestamp -= min_corrected_pts; > + } > + } > + > // Update av stream length > st->duration = edit_list_dts_entry_end - start_dts; > > diff --git a/tests/ref/fate/h264-twofields-packet b/tests/ref/fate/h264-twofields-packet > index 4cff0a15bd..db12498f15 100644 > --- a/tests/ref/fate/h264-twofields-packet > +++ b/tests/ref/fate/h264-twofields-packet > @@ -3,33 +3,33 @@ > #codec_id 0: rawvideo > #dimensions 0: 1920x1080 > #sar 0: 1/1 > -0, 0, 0, 1, 3110400, 0x40d65f69 > -0, 1, 1, 1, 3110400, 0xdcbc50bf > -0, 2, 2, 1, 3110400, 0x73a2276a > -0, 3, 3, 1, 3110400, 0x84a2b3c6 > -0, 4, 4, 1, 3110400, 0x7cf3b570 > -0, 5, 5, 1, 3110400, 0xa2d1e03a > -0, 6, 6, 1, 3110400, 0x03220fb1 > -0, 7, 7, 1, 3110400, 0x89cd526a > -0, 8, 8, 1, 3110400, 0xbb4b7531 > -0, 9, 9, 1, 3110400, 0x0a69f053 > -0, 10, 10, 1, 3110400, 0x0187994b > -0, 11, 11, 1, 3110400, 0x26ed49fa > -0, 12, 12, 1, 3110400, 0xbe8966d4 > -0, 13, 13, 1, 3110400, 0x248d203c > -0, 14, 14, 1, 3110400, 0x3139c754 > -0, 15, 15, 1, 3110400, 0xf22380c4 > -0, 16, 16, 1, 3110400, 0x3e00dcc1 > -0, 17, 17, 1, 3110400, 0x8cbe2483 > -0, 18, 18, 1, 3110400, 0x6951cd63 > -0, 19, 19, 1, 3110400, 0x36aca4c5 > -0, 20, 20, 1, 3110400, 0x4d4f6fbe > -0, 21, 21, 1, 3110400, 0x997247aa > -0, 22, 22, 1, 3110400, 0x0fd40e06 > -0, 23, 23, 1, 3110400, 0xa10d2d67 > -0, 24, 24, 1, 3110400, 0x87c481da > -0, 25, 25, 1, 3110400, 0xe3dca3cd > -0, 26, 26, 1, 3110400, 0x5f77b078 > -0, 27, 27, 1, 3110400, 0xf1ddd098 > -0, 28, 28, 1, 3110400, 0xedcd1754 > -0, 29, 29, 1, 3110400, 0x14ac7153 > +0, 0, 0, 1, 3110400, 0x48e39acd > +0, 1, 1, 1, 3110400, 0x40d65f69 > +0, 2, 2, 1, 3110400, 0xdcbc50bf > +0, 3, 3, 1, 3110400, 0x73a2276a > +0, 4, 4, 1, 3110400, 0x84a2b3c6 > +0, 5, 5, 1, 3110400, 0x7cf3b570 > +0, 6, 6, 1, 3110400, 0xa2d1e03a > +0, 7, 7, 1, 3110400, 0x03220fb1 > +0, 8, 8, 1, 3110400, 0x89cd526a > +0, 9, 9, 1, 3110400, 0xbb4b7531 > +0, 10, 10, 1, 3110400, 0x0a69f053 > +0, 11, 11, 1, 3110400, 0x0187994b > +0, 12, 12, 1, 3110400, 0x26ed49fa > +0, 13, 13, 1, 3110400, 0xbe8966d4 > +0, 14, 14, 1, 3110400, 0x248d203c > +0, 15, 15, 1, 3110400, 0x3139c754 > +0, 16, 16, 1, 3110400, 0xf22380c4 > +0, 17, 17, 1, 3110400, 0x3e00dcc1 > +0, 18, 18, 1, 3110400, 0x8cbe2483 > +0, 19, 19, 1, 3110400, 0x6951cd63 > +0, 20, 20, 1, 3110400, 0x36aca4c5 > +0, 21, 21, 1, 3110400, 0x4d4f6fbe > +0, 22, 22, 1, 3110400, 0x997247aa > +0, 23, 23, 1, 3110400, 0x0fd40e06 > +0, 24, 24, 1, 3110400, 0xa10d2d67 > +0, 25, 25, 1, 3110400, 0x87c481da > +0, 26, 26, 1, 3110400, 0xe3dca3cd > +0, 27, 27, 1, 3110400, 0x5f77b078 > +0, 28, 28, 1, 3110400, 0xf1ddd098 > +0, 29, 29, 1, 3110400, 0xedcd1754 Thanks for the fix, but is there any explanation why this happens? And why does the FATE test change?
> - // Offset the DTS by ctts[0] to make the PTS of the first frame 0 > - if (ctts_data_old && ctts_count_old > 0) { > - edit_list_dts_entry_end -= ctts_data_old[0].duration; > - av_log(mov->fc, AV_LOG_DEBUG, "Offset DTS by ctts[%d].duration: %d\n", 0, ctts_data_old[0].duration); > + av_log(mov->fc, AV_LOG_DEBUG, "Shifting DTS by %d because of negative CTTS.\n", msc->dts_shift); > } The above lines were the cause of making the first frame PTS of videos starting with B-frames negative. If the videos starts with B frame, then the minimum composition time as computed by stts + ctts will be non-zero. Hence we need to shift the DTS, so that the first pts is zero. This was the intention of that code-block. However it was subtracting by the wrong amount. For example, for one of the videos in the bug nonFormatted.mp4 we have stts: sample_count duration 960 1001 ctts: sample_count duration 1 3003 2 0 1 3003 .... The resulting composition times are : 3003, 1001, 2002, 6006, ... Hence the minimum composition time or PTS is 1001, which should be used to offset DTS. However the code block was wrongly using ctts[0] which is 3003 . Hence the PTS was negative. About the fate test expectations, fate-suite/h264/twofields_packet.mp4 is a similar file starting with 2 Bframes. Before this change the PTS of first two B-frames was -6006 and -3003, and I am guessing one of them got dropped when being decoded and remuxed to the framecrc before, and now it is not being dropped but I dont understand why. On Sat, Jun 3, 2017 at 5:40 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Fri, 2 Jun 2017 18:17:22 -0700 > Sasi Inguva <isasi-at-google.com@ffmpeg.org> wrote: > > > Fixes t/6421 > > > > Signed-off-by: Sasi Inguva <isasi@google.com> > > --- > > libavformat/mov.c | 57 ++++++++++++++++++++++++------ > ---- > > tests/ref/fate/h264-twofields-packet | 60 > ++++++++++++++++++------------------ > > 2 files changed, 70 insertions(+), 47 deletions(-) > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c > > index 3845e63b53..d7d64c3361 100644 > > --- a/libavformat/mov.c > > +++ b/libavformat/mov.c > > @@ -3039,6 +3039,9 @@ static void mov_fix_index(MOVContext *mov, > AVStream *st) > > int64_t edit_list_dts_entry_end = 0; > > int64_t edit_list_start_ctts_sample = 0; > > int64_t curr_cts; > > + int64_t curr_ctts = 0; > > + int64_t min_corrected_pts = -1; > > + int64_t empty_edits_sum_duration = 0; > > int64_t edit_list_index = 0; > > int64_t index; > > int64_t index_ctts_count; > > @@ -3053,6 +3056,7 @@ static void mov_fix_index(MOVContext *mov, > AVStream *st) > > int first_non_zero_audio_edit = -1; > > int packet_skip_samples = 0; > > MOVIndexRange *current_index_range; > > + int i; > > > > if (!msc->elst_data || msc->elst_count <= 0 || nb_old <= 0) { > > return; > > @@ -3080,13 +3084,9 @@ static void mov_fix_index(MOVContext *mov, > AVStream *st) > > > > // If the dts_shift is positive (in case of negative ctts values in > mov), > > // then negate the DTS by dts_shift > > - if (msc->dts_shift > 0) > > + if (msc->dts_shift > 0) { > > edit_list_dts_entry_end -= msc->dts_shift; > > - > > - // Offset the DTS by ctts[0] to make the PTS of the first frame 0 > > - if (ctts_data_old && ctts_count_old > 0) { > > - edit_list_dts_entry_end -= ctts_data_old[0].duration; > > - av_log(mov->fc, AV_LOG_DEBUG, "Offset DTS by ctts[%d].duration: > %d\n", 0, ctts_data_old[0].duration); > > + av_log(mov->fc, AV_LOG_DEBUG, "Shifting DTS by %d because of > negative CTTS.\n", msc->dts_shift); > > } > > > > start_dts = edit_list_dts_entry_end; > > @@ -3100,6 +3100,7 @@ static void mov_fix_index(MOVContext *mov, > AVStream *st) > > edit_list_dts_entry_end += edit_list_duration; > > num_discarded_begin = 0; > > if (edit_list_media_time == -1) { > > + empty_edits_sum_duration += edit_list_duration; > > continue; > > } > > > > @@ -3179,11 +3180,13 @@ static void mov_fix_index(MOVContext *mov, > AVStream *st) > > > > // frames (pts) before or after edit list > > curr_cts = current->timestamp + msc->dts_shift; > > + curr_ctts = 0; > > > > if (ctts_data_old && ctts_index_old < ctts_count_old) { > > - av_log(mov->fc, AV_LOG_DEBUG, "shifted frame pts, > curr_cts: %"PRId64" @ %"PRId64", ctts: %d, ctts_count: %"PRId64"\n", > > - curr_cts, ctts_index_old, > ctts_data_old[ctts_index_old].duration, ctts_count_old); > > - curr_cts += ctts_data_old[ctts_index_old].duration; > > + curr_ctts = ctts_data_old[ctts_index_old].duration; > > + av_log(mov->fc, AV_LOG_DEBUG, "stts: %"PRId64" ctts: > %"PRId64", ctts_index: %"PRId64", ctts_count: %"PRId64"\n", > > + curr_cts, curr_ctts, ctts_index_old, > ctts_count_old); > > + curr_cts += curr_ctts; > > ctts_sample_old++; > > if (ctts_sample_old == ctts_data_old[ctts_index_old].count) > { > > if (add_ctts_entry(&msc->ctts_data, > &msc->ctts_count, > > @@ -3244,14 +3247,21 @@ static void mov_fix_index(MOVContext *mov, > AVStream *st) > > } > > } > > } > > - } else if (edit_list_start_encountered == 0) { > > - edit_list_start_encountered = 1; > > - // Make timestamps strictly monotonically increasing > for audio, by rewriting timestamps for > > - // discarded packets. > > - if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && > frame_duration_buffer) { > > - fix_index_entry_timestamps(st, > st->nb_index_entries, edit_list_dts_counter, > > - frame_duration_buffer, > num_discarded_begin); > > - av_freep(&frame_duration_buffer); > > + } else { > > + if (min_corrected_pts < 0) { > > + min_corrected_pts = edit_list_dts_counter + > curr_ctts + msc->dts_shift; > > + } else { > > + min_corrected_pts = FFMIN(min_corrected_pts, > edit_list_dts_counter + curr_ctts + msc->dts_shift); > > + } > > + if (edit_list_start_encountered == 0) { > > + edit_list_start_encountered = 1; > > + // Make timestamps strictly monotonically > increasing for audio, by rewriting timestamps for > > + // discarded packets. > > + if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO > && frame_duration_buffer) { > > + fix_index_entry_timestamps(st, > st->nb_index_entries, edit_list_dts_counter, > > + > frame_duration_buffer, num_discarded_begin); > > + av_freep(&frame_duration_buffer); > > + } > > } > > } > > > > @@ -3292,6 +3302,19 @@ static void mov_fix_index(MOVContext *mov, > AVStream *st) > > } > > } > > } > > + // If there are empty edits, then min_corrected_pts might be > positive intentionally. So we subtract the > > + // sum duration of emtpy edits here. > > + min_corrected_pts -= empty_edits_sum_duration; > > + > > + // If the minimum pts turns out to be greater than zero after > fixing the index, then we subtract the > > + // dts by that amount to make the first pts zero. > > + if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && > min_corrected_pts > 0) { > > + av_log(mov->fc, AV_LOG_DEBUG, "Offset DTS by %"PRId64" to make > first pts zero.\n", min_corrected_pts); > > + for (i = 0; i < st->nb_index_entries; ++i) { > > + st->index_entries[i].timestamp -= min_corrected_pts; > > + } > > + } > > + > > // Update av stream length > > st->duration = edit_list_dts_entry_end - start_dts; > > > > diff --git a/tests/ref/fate/h264-twofields-packet b/tests/ref/fate/h264- > twofields-packet > > index 4cff0a15bd..db12498f15 100644 > > --- a/tests/ref/fate/h264-twofields-packet > > +++ b/tests/ref/fate/h264-twofields-packet > > @@ -3,33 +3,33 @@ > > #codec_id 0: rawvideo > > #dimensions 0: 1920x1080 > > #sar 0: 1/1 > > -0, 0, 0, 1, 3110400, 0x40d65f69 > > -0, 1, 1, 1, 3110400, 0xdcbc50bf > > -0, 2, 2, 1, 3110400, 0x73a2276a > > -0, 3, 3, 1, 3110400, 0x84a2b3c6 > > -0, 4, 4, 1, 3110400, 0x7cf3b570 > > -0, 5, 5, 1, 3110400, 0xa2d1e03a > > -0, 6, 6, 1, 3110400, 0x03220fb1 > > -0, 7, 7, 1, 3110400, 0x89cd526a > > -0, 8, 8, 1, 3110400, 0xbb4b7531 > > -0, 9, 9, 1, 3110400, 0x0a69f053 > > -0, 10, 10, 1, 3110400, 0x0187994b > > -0, 11, 11, 1, 3110400, 0x26ed49fa > > -0, 12, 12, 1, 3110400, 0xbe8966d4 > > -0, 13, 13, 1, 3110400, 0x248d203c > > -0, 14, 14, 1, 3110400, 0x3139c754 > > -0, 15, 15, 1, 3110400, 0xf22380c4 > > -0, 16, 16, 1, 3110400, 0x3e00dcc1 > > -0, 17, 17, 1, 3110400, 0x8cbe2483 > > -0, 18, 18, 1, 3110400, 0x6951cd63 > > -0, 19, 19, 1, 3110400, 0x36aca4c5 > > -0, 20, 20, 1, 3110400, 0x4d4f6fbe > > -0, 21, 21, 1, 3110400, 0x997247aa > > -0, 22, 22, 1, 3110400, 0x0fd40e06 > > -0, 23, 23, 1, 3110400, 0xa10d2d67 > > -0, 24, 24, 1, 3110400, 0x87c481da > > -0, 25, 25, 1, 3110400, 0xe3dca3cd > > -0, 26, 26, 1, 3110400, 0x5f77b078 > > -0, 27, 27, 1, 3110400, 0xf1ddd098 > > -0, 28, 28, 1, 3110400, 0xedcd1754 > > -0, 29, 29, 1, 3110400, 0x14ac7153 > > +0, 0, 0, 1, 3110400, 0x48e39acd > > +0, 1, 1, 1, 3110400, 0x40d65f69 > > +0, 2, 2, 1, 3110400, 0xdcbc50bf > > +0, 3, 3, 1, 3110400, 0x73a2276a > > +0, 4, 4, 1, 3110400, 0x84a2b3c6 > > +0, 5, 5, 1, 3110400, 0x7cf3b570 > > +0, 6, 6, 1, 3110400, 0xa2d1e03a > > +0, 7, 7, 1, 3110400, 0x03220fb1 > > +0, 8, 8, 1, 3110400, 0x89cd526a > > +0, 9, 9, 1, 3110400, 0xbb4b7531 > > +0, 10, 10, 1, 3110400, 0x0a69f053 > > +0, 11, 11, 1, 3110400, 0x0187994b > > +0, 12, 12, 1, 3110400, 0x26ed49fa > > +0, 13, 13, 1, 3110400, 0xbe8966d4 > > +0, 14, 14, 1, 3110400, 0x248d203c > > +0, 15, 15, 1, 3110400, 0x3139c754 > > +0, 16, 16, 1, 3110400, 0xf22380c4 > > +0, 17, 17, 1, 3110400, 0x3e00dcc1 > > +0, 18, 18, 1, 3110400, 0x8cbe2483 > > +0, 19, 19, 1, 3110400, 0x6951cd63 > > +0, 20, 20, 1, 3110400, 0x36aca4c5 > > +0, 21, 21, 1, 3110400, 0x4d4f6fbe > > +0, 22, 22, 1, 3110400, 0x997247aa > > +0, 23, 23, 1, 3110400, 0x0fd40e06 > > +0, 24, 24, 1, 3110400, 0xa10d2d67 > > +0, 25, 25, 1, 3110400, 0x87c481da > > +0, 26, 26, 1, 3110400, 0xe3dca3cd > > +0, 27, 27, 1, 3110400, 0x5f77b078 > > +0, 28, 28, 1, 3110400, 0xf1ddd098 > > +0, 29, 29, 1, 3110400, 0xedcd1754 > > Thanks for the fix, but is there any explanation why this happens? And > why does the FATE test change? > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Sat, 3 Jun 2017 19:31:37 -0700 Sasi Inguva <isasi-at-google.com@ffmpeg.org> wrote: > > - // Offset the DTS by ctts[0] to make the PTS of the first frame 0 > > - if (ctts_data_old && ctts_count_old > 0) { > > - edit_list_dts_entry_end -= ctts_data_old[0].duration; > > - av_log(mov->fc, AV_LOG_DEBUG, "Offset DTS by ctts[%d].duration: > %d\n", 0, ctts_data_old[0].duration); > > + av_log(mov->fc, AV_LOG_DEBUG, "Shifting DTS by %d because of > negative CTTS.\n", msc->dts_shift); > > } > > The above lines were the cause of making the first frame PTS of videos > starting with B-frames negative. If the videos starts with B frame, then > the minimum composition time as computed by stts + ctts will be non-zero. > Hence we need to shift the DTS, so that the first pts is zero. This was the > intention of that code-block. However it was subtracting by the wrong > amount. > For example, for one of the videos in the bug nonFormatted.mp4 we have > stts: > sample_count duration > 960 1001 > ctts: > sample_count duration > 1 3003 > 2 0 > 1 3003 > .... > > The resulting composition times are : 3003, 1001, 2002, 6006, ... > Hence the minimum composition time or PTS is 1001, which should be used to > offset DTS. However the code block was wrongly using ctts[0] which is 3003 > . Hence the PTS was negative. > > About the fate test expectations, fate-suite/h264/twofields_packet.mp4 is a > similar file starting with 2 Bframes. Before this change the PTS of first > two B-frames was -6006 and -3003, and I am guessing one of them got dropped > when being decoded and remuxed to the framecrc before, and now it is not > being dropped but I dont understand why. All of this should go into the commit message. Can't judge the correctness of the actual code.
Got it. Added to the description. On Tue, Jun 6, 2017 at 6:51 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Sat, 3 Jun 2017 19:31:37 -0700 > Sasi Inguva <isasi-at-google.com@ffmpeg.org> wrote: > > > > - // Offset the DTS by ctts[0] to make the PTS of the first frame 0 > > > - if (ctts_data_old && ctts_count_old > 0) { > > > - edit_list_dts_entry_end -= ctts_data_old[0].duration; > > > - av_log(mov->fc, AV_LOG_DEBUG, "Offset DTS by > ctts[%d].duration: > > %d\n", 0, ctts_data_old[0].duration); > > > + av_log(mov->fc, AV_LOG_DEBUG, "Shifting DTS by %d because of > > negative CTTS.\n", msc->dts_shift); > > > } > > > > The above lines were the cause of making the first frame PTS of videos > > starting with B-frames negative. If the videos starts with B frame, then > > the minimum composition time as computed by stts + ctts will be non-zero. > > Hence we need to shift the DTS, so that the first pts is zero. This was > the > > intention of that code-block. However it was subtracting by the wrong > > amount. > > For example, for one of the videos in the bug nonFormatted.mp4 we have > > stts: > > sample_count duration > > 960 1001 > > ctts: > > sample_count duration > > 1 3003 > > 2 0 > > 1 3003 > > .... > > > > The resulting composition times are : 3003, 1001, 2002, 6006, ... > > Hence the minimum composition time or PTS is 1001, which should be used > to > > offset DTS. However the code block was wrongly using ctts[0] which is > 3003 > > . Hence the PTS was negative. > > > > About the fate test expectations, fate-suite/h264/twofields_packet.mp4 > is a > > similar file starting with 2 Bframes. Before this change the PTS of first > > two B-frames was -6006 and -3003, and I am guessing one of them got > dropped > > when being decoded and remuxed to the framecrc before, and now it is not > > being dropped but I dont understand why. > > All of this should go into the commit message. > > Can't judge the correctness of the actual code. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
Ping! On Jun 6, 2017 11:17 AM, "Sasi Inguva" <isasi@google.com> wrote: > Got it. Added to the description. > > On Tue, Jun 6, 2017 at 6:51 AM, wm4 <nfxjfg@googlemail.com> wrote: > >> On Sat, 3 Jun 2017 19:31:37 -0700 >> Sasi Inguva <isasi-at-google.com@ffmpeg.org> wrote: >> >> > > - // Offset the DTS by ctts[0] to make the PTS of the first frame 0 >> > > - if (ctts_data_old && ctts_count_old > 0) { >> > > - edit_list_dts_entry_end -= ctts_data_old[0].duration; >> > > - av_log(mov->fc, AV_LOG_DEBUG, "Offset DTS by >> ctts[%d].duration: >> > %d\n", 0, ctts_data_old[0].duration); >> > > + av_log(mov->fc, AV_LOG_DEBUG, "Shifting DTS by %d because of >> > negative CTTS.\n", msc->dts_shift); >> > > } >> > >> > The above lines were the cause of making the first frame PTS of videos >> > starting with B-frames negative. If the videos starts with B frame, then >> > the minimum composition time as computed by stts + ctts will be >> non-zero. >> > Hence we need to shift the DTS, so that the first pts is zero. This was >> the >> > intention of that code-block. However it was subtracting by the wrong >> > amount. >> > For example, for one of the videos in the bug nonFormatted.mp4 we have >> > stts: >> > sample_count duration >> > 960 1001 >> > ctts: >> > sample_count duration >> > 1 3003 >> > 2 0 >> > 1 3003 >> > .... >> > >> > The resulting composition times are : 3003, 1001, 2002, 6006, ... >> > Hence the minimum composition time or PTS is 1001, which should be used >> to >> > offset DTS. However the code block was wrongly using ctts[0] which is >> 3003 >> > . Hence the PTS was negative. >> > >> > About the fate test expectations, fate-suite/h264/twofields_packet.mp4 >> is a >> > similar file starting with 2 Bframes. Before this change the PTS of >> first >> > two B-frames was -6006 and -3003, and I am guessing one of them got >> dropped >> > when being decoded and remuxed to the framecrc before, and now it is >> not >> > being dropped but I dont understand why. >> >> All of this should go into the commit message. >> >> Can't judge the correctness of the actual code. >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > >
On Thu, 8 Jun 2017 12:28:41 -0700 Sasi Inguva <isasi-at-google.com@ffmpeg.org> wrote: > Ping! > If nobody has another comment, I guess I can push it tomorrow or next week.
diff --git a/libavformat/mov.c b/libavformat/mov.c index 3845e63b53..d7d64c3361 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -3039,6 +3039,9 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) int64_t edit_list_dts_entry_end = 0; int64_t edit_list_start_ctts_sample = 0; int64_t curr_cts; + int64_t curr_ctts = 0; + int64_t min_corrected_pts = -1; + int64_t empty_edits_sum_duration = 0; int64_t edit_list_index = 0; int64_t index; int64_t index_ctts_count; @@ -3053,6 +3056,7 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) int first_non_zero_audio_edit = -1; int packet_skip_samples = 0; MOVIndexRange *current_index_range; + int i; if (!msc->elst_data || msc->elst_count <= 0 || nb_old <= 0) { return; @@ -3080,13 +3084,9 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) // If the dts_shift is positive (in case of negative ctts values in mov), // then negate the DTS by dts_shift - if (msc->dts_shift > 0) + if (msc->dts_shift > 0) { edit_list_dts_entry_end -= msc->dts_shift; - - // Offset the DTS by ctts[0] to make the PTS of the first frame 0 - if (ctts_data_old && ctts_count_old > 0) { - edit_list_dts_entry_end -= ctts_data_old[0].duration; - av_log(mov->fc, AV_LOG_DEBUG, "Offset DTS by ctts[%d].duration: %d\n", 0, ctts_data_old[0].duration); + av_log(mov->fc, AV_LOG_DEBUG, "Shifting DTS by %d because of negative CTTS.\n", msc->dts_shift); } start_dts = edit_list_dts_entry_end; @@ -3100,6 +3100,7 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) edit_list_dts_entry_end += edit_list_duration; num_discarded_begin = 0; if (edit_list_media_time == -1) { + empty_edits_sum_duration += edit_list_duration; continue; } @@ -3179,11 +3180,13 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) // frames (pts) before or after edit list curr_cts = current->timestamp + msc->dts_shift; + curr_ctts = 0; if (ctts_data_old && ctts_index_old < ctts_count_old) { - av_log(mov->fc, AV_LOG_DEBUG, "shifted frame pts, curr_cts: %"PRId64" @ %"PRId64", ctts: %d, ctts_count: %"PRId64"\n", - curr_cts, ctts_index_old, ctts_data_old[ctts_index_old].duration, ctts_count_old); - curr_cts += ctts_data_old[ctts_index_old].duration; + curr_ctts = ctts_data_old[ctts_index_old].duration; + av_log(mov->fc, AV_LOG_DEBUG, "stts: %"PRId64" ctts: %"PRId64", ctts_index: %"PRId64", ctts_count: %"PRId64"\n", + curr_cts, curr_ctts, ctts_index_old, ctts_count_old); + curr_cts += curr_ctts; ctts_sample_old++; if (ctts_sample_old == ctts_data_old[ctts_index_old].count) { if (add_ctts_entry(&msc->ctts_data, &msc->ctts_count, @@ -3244,14 +3247,21 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) } } } - } else if (edit_list_start_encountered == 0) { - edit_list_start_encountered = 1; - // Make timestamps strictly monotonically increasing for audio, by rewriting timestamps for - // discarded packets. - if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && frame_duration_buffer) { - fix_index_entry_timestamps(st, st->nb_index_entries, edit_list_dts_counter, - frame_duration_buffer, num_discarded_begin); - av_freep(&frame_duration_buffer); + } else { + if (min_corrected_pts < 0) { + min_corrected_pts = edit_list_dts_counter + curr_ctts + msc->dts_shift; + } else { + min_corrected_pts = FFMIN(min_corrected_pts, edit_list_dts_counter + curr_ctts + msc->dts_shift); + } + if (edit_list_start_encountered == 0) { + edit_list_start_encountered = 1; + // Make timestamps strictly monotonically increasing for audio, by rewriting timestamps for + // discarded packets. + if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && frame_duration_buffer) { + fix_index_entry_timestamps(st, st->nb_index_entries, edit_list_dts_counter, + frame_duration_buffer, num_discarded_begin); + av_freep(&frame_duration_buffer); + } } } @@ -3292,6 +3302,19 @@ static void mov_fix_index(MOVContext *mov, AVStream *st) } } } + // If there are empty edits, then min_corrected_pts might be positive intentionally. So we subtract the + // sum duration of emtpy edits here. + min_corrected_pts -= empty_edits_sum_duration; + + // If the minimum pts turns out to be greater than zero after fixing the index, then we subtract the + // dts by that amount to make the first pts zero. + if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && min_corrected_pts > 0) { + av_log(mov->fc, AV_LOG_DEBUG, "Offset DTS by %"PRId64" to make first pts zero.\n", min_corrected_pts); + for (i = 0; i < st->nb_index_entries; ++i) { + st->index_entries[i].timestamp -= min_corrected_pts; + } + } + // Update av stream length st->duration = edit_list_dts_entry_end - start_dts; diff --git a/tests/ref/fate/h264-twofields-packet b/tests/ref/fate/h264-twofields-packet index 4cff0a15bd..db12498f15 100644 --- a/tests/ref/fate/h264-twofields-packet +++ b/tests/ref/fate/h264-twofields-packet @@ -3,33 +3,33 @@ #codec_id 0: rawvideo #dimensions 0: 1920x1080 #sar 0: 1/1 -0, 0, 0, 1, 3110400, 0x40d65f69 -0, 1, 1, 1, 3110400, 0xdcbc50bf -0, 2, 2, 1, 3110400, 0x73a2276a -0, 3, 3, 1, 3110400, 0x84a2b3c6 -0, 4, 4, 1, 3110400, 0x7cf3b570 -0, 5, 5, 1, 3110400, 0xa2d1e03a -0, 6, 6, 1, 3110400, 0x03220fb1 -0, 7, 7, 1, 3110400, 0x89cd526a -0, 8, 8, 1, 3110400, 0xbb4b7531 -0, 9, 9, 1, 3110400, 0x0a69f053 -0, 10, 10, 1, 3110400, 0x0187994b -0, 11, 11, 1, 3110400, 0x26ed49fa -0, 12, 12, 1, 3110400, 0xbe8966d4 -0, 13, 13, 1, 3110400, 0x248d203c -0, 14, 14, 1, 3110400, 0x3139c754 -0, 15, 15, 1, 3110400, 0xf22380c4 -0, 16, 16, 1, 3110400, 0x3e00dcc1 -0, 17, 17, 1, 3110400, 0x8cbe2483 -0, 18, 18, 1, 3110400, 0x6951cd63 -0, 19, 19, 1, 3110400, 0x36aca4c5 -0, 20, 20, 1, 3110400, 0x4d4f6fbe -0, 21, 21, 1, 3110400, 0x997247aa -0, 22, 22, 1, 3110400, 0x0fd40e06 -0, 23, 23, 1, 3110400, 0xa10d2d67 -0, 24, 24, 1, 3110400, 0x87c481da -0, 25, 25, 1, 3110400, 0xe3dca3cd -0, 26, 26, 1, 3110400, 0x5f77b078 -0, 27, 27, 1, 3110400, 0xf1ddd098 -0, 28, 28, 1, 3110400, 0xedcd1754 -0, 29, 29, 1, 3110400, 0x14ac7153 +0, 0, 0, 1, 3110400, 0x48e39acd +0, 1, 1, 1, 3110400, 0x40d65f69 +0, 2, 2, 1, 3110400, 0xdcbc50bf +0, 3, 3, 1, 3110400, 0x73a2276a +0, 4, 4, 1, 3110400, 0x84a2b3c6 +0, 5, 5, 1, 3110400, 0x7cf3b570 +0, 6, 6, 1, 3110400, 0xa2d1e03a +0, 7, 7, 1, 3110400, 0x03220fb1 +0, 8, 8, 1, 3110400, 0x89cd526a +0, 9, 9, 1, 3110400, 0xbb4b7531 +0, 10, 10, 1, 3110400, 0x0a69f053 +0, 11, 11, 1, 3110400, 0x0187994b +0, 12, 12, 1, 3110400, 0x26ed49fa +0, 13, 13, 1, 3110400, 0xbe8966d4 +0, 14, 14, 1, 3110400, 0x248d203c +0, 15, 15, 1, 3110400, 0x3139c754 +0, 16, 16, 1, 3110400, 0xf22380c4 +0, 17, 17, 1, 3110400, 0x3e00dcc1 +0, 18, 18, 1, 3110400, 0x8cbe2483 +0, 19, 19, 1, 3110400, 0x6951cd63 +0, 20, 20, 1, 3110400, 0x36aca4c5 +0, 21, 21, 1, 3110400, 0x4d4f6fbe +0, 22, 22, 1, 3110400, 0x997247aa +0, 23, 23, 1, 3110400, 0x0fd40e06 +0, 24, 24, 1, 3110400, 0xa10d2d67 +0, 25, 25, 1, 3110400, 0x87c481da +0, 26, 26, 1, 3110400, 0xe3dca3cd +0, 27, 27, 1, 3110400, 0x5f77b078 +0, 28, 28, 1, 3110400, 0xf1ddd098 +0, 29, 29, 1, 3110400, 0xedcd1754
Fixes t/6421 Signed-off-by: Sasi Inguva <isasi@google.com> --- libavformat/mov.c | 57 ++++++++++++++++++++++++---------- tests/ref/fate/h264-twofields-packet | 60 ++++++++++++++++++------------------ 2 files changed, 70 insertions(+), 47 deletions(-)