On 02/22/2017 03:41 AM, Martin Storsjö wrote:
> On Tue, 21 Feb 2017, John Stebbins wrote:
>
>> On 02/21/2017 02:54 PM, Martin Storsjö wrote:
>>> On Tue, 21 Feb 2017, John Stebbins wrote:
>>>
>>>> initial_padding was getting added to the edit list indirectly due to
>>>> initial negative dts. But in cases where the audio is delayed,
>>>> all or part of initial_padding would be unaccounted for. This patch
>>>> makes initial_padding explicit.
>>>> ---
>>>> libavformat/movenc.c | 58
>>>> +++++++++++++++++++++++++++++++++++++---------------
>>>> 1 file changed, 42 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>>> index 689291d..d60e194 100644
>>>> --- a/libavformat/movenc.c
>>>> +++ b/libavformat/movenc.c
>>>> @@ -1698,9 +1698,31 @@ static int mov_write_edts_tag(AVIOContext *pb,
>>>> MOVMuxContext *mov,
>>>> track->timescale, AV_ROUND_UP);
>>>> int version = duration < INT32_MAX ? 0 : 1;
>>>> int entry_size, entry_count, size;
>>>> - int64_t delay, start_ct = track->start_cts;
>>>> - delay = av_rescale_rnd(track->start_dts + start_ct, MOV_TIMESCALE,
>>>> - track->timescale, AV_ROUND_DOWN);
>>>> + int64_t delay;
>>>> + int64_t mediatime;
>>>> + int64_t skip = 0;
>>>> +
>>>> + delay = track->start_dts + track->start_cts;
>>>> +
>>>> + if (track->par->codec_type == AVMEDIA_TYPE_AUDIO &&
>>>> + track->par->initial_padding > 0) {
>>>> + /* Adjust delay so that initial_padding gets recorded in the
>>>> + * MediaTime of an edit list entry even in the case that
>>>> + * delay is positive. I.e. we don't want initial_padding to be
>>>> + * absorbed and hidden in the delay. MediaTime must contain
>>>> + * initial_padding in order to know where the actual media
>>>> + * timeline begins. A player should drop samples until MediaTime
>>>> + * is reached */
>>>> + delay += av_rescale_rnd(track->par->initial_padding,
>>>> track->timescale,
>>>> + track->par->sample_rate, AV_ROUND_UP);
>>>> + skip = av_rescale_rnd(track->par->initial_padding,
>>>> + track->timescale,
>>>> + track->par->sample_rate, AV_ROUND_DOWN);
>>>> + }
>>>> + /* rescale delay, this was not done earlier to minimize rounding
>>>> errors */
>>>> + delay = av_rescale_rnd(delay, MOV_TIMESCALE,
>>>> + track->timescale, AV_ROUND_NEAR_INF);
>>> Nice; I thought about pointing this out in the previous review but
>>> skipped, since it would at most mean an 1 ms rounding error. But this way
>>> it's even better, especially since the timescale in most cases will be
>>> equal to the sample rate.
>>>
>>>> +
>>>> version |= delay < INT32_MAX ? 0 : 1;
>>>>
>>>> entry_size = (version == 1) ? 20 : 12;
>>>> @@ -1731,33 +1753,37 @@ static int mov_write_edts_tag(AVIOContext *pb,
>>>> MOVMuxContext *mov,
>>>> }
>>>> avio_wb32(pb, 0x00010000);
>>>> } else {
>>>> - /* Avoid accidentally ending up with start_ct = -1 which has got a
>>>> - * special meaning. Normally start_ct should end up positive or
>>>> zero
>>>> - * here, but use FFMIN in case dts is a a small positive integer
>>>> - * rounded to 0 when represented in MOV_TIMESCALE units. */
>>>> - start_ct = -FFMIN(track->start_dts, 0);
>>>> - /* Note, this delay is calculated from the pts of the first
>>>> sample,
>>>> - * ensuring that we don't reduce the duration for cases with
>>>> - * dts<0 pts=0. */
>>>> - duration += delay;
>>>> + /* Skip the larger of initial_padding or the initial -PTS.
>>> This is good, this makes it very clear what it does.
>>>
>>>> + * Skipping -PTS when PTS is negative ensures that timestamps
>>>> + * can be represented with positive values in stts and ctts. */
>>> This still has me confused though - I don't see what that has to do with
>>> this code here.
>> Better wording is still required I guess. stts and ctts will be positive
>> regardless, but if the edit list entry is
>> *not* set to -PTS the resulting timestamps during decode will be wrong. The
>> only way to get correct timestamps without
>> setting -PTS as MediaTime would be something like your negative CTS patch.
> Yup. Given this, I'd be ok with reducing this comment just to the first
> sentence - that seems like enough to me.
>
>>> Also see the patchset I posted the other day, about adding the
>>> negative_cts_offsets flag. In mov/mp4, it's ok with negative ctts values,
>>> which makes it possible to omit the edit lists for normal video with
>>> b-frames.
>> Yes, I did see this patch set.
> Ok, great. If this goes in first, I'll need to update patch 2/3 to check
> for "start_dts || initial_padding".I actually should have added initial_padding to this test since it's possible for an audio track to be delayed by exactly the initial_padding which would result in start_dts = 0. So I'll update my patch for this. -- John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
signature.asc
Description: OpenPGP digital signature
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
