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. > 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. > > The rest of this patch seems fine (still) - you seems to be doing a lot of > practical testing of it, which gives me confidence. I assume/hope you've > tested (either manually or later via the movenc unit test) that it > preserves the current behaviour for video tracks that start with a > positive offset. I assume you mean delay here where an empty edit is inserted. I've tested video with positive delay, audio with positive delay and both with 0 delay. There is also another related patch on the mailing list now that extracts initial_padding from the file while reading. So I've tested writing->reading->writing->reading ... multiple generations and verified for every generation that streams stay in sync, start consistently at the same time and remain the same duration. > // Martin > _______________________________________________ > libav-devel mailing list > [email protected] > https://lists.libav.org/mailman/listinfo/libav-devel -- 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
