On 02/25/2017 03:09 AM, Martin Storsjö wrote:
> On Fri, 24 Feb 2017, John Stebbins wrote:
>
>> On 02/24/2017 03:13 PM, Martin Storsjö wrote:
>>> On Fri, 24 Feb 2017, John Stebbins wrote:
>>>
>>>> On 02/24/2017 02:21 PM, Martin Storsjö wrote:
>>>>> On Fri, 24 Feb 2017, John Stebbins wrote:
>>>>>
>>>>>> This recordes the number of AAC frames that must be decoded before valid
>>>>>> audio is decoded.
>>>>>>
>>>>>> It also signals to Apple tools that the encoder delay is explicitly set
>>>>>> in the edit list. If the roll sample group is omitted, Apply tools will
>>>>>> apply an implicit rule to remove encoder delay which would result in
>>>>>> removing the delay twice.
>>>>>> ---
>>>>>> libavformat/movenc.c | 63
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> tests/ref/fate/movenc | 32 +++++++++++++-------------
>>>>>> 2 files changed, 79 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>>>>> index 4d351b7..a26dcf7 100644
>>>>>> --- a/libavformat/movenc.c
>>>>>> +++ b/libavformat/movenc.c
>>>>>> @@ -136,6 +136,54 @@ static int mov_write_stco_tag(AVIOContext *pb,
>>>>>> MOVTrack *track)
>>>>>> return update_size(pb, pos);
>>>>>> }
>>>>>>
>>>>>> +static int mov_write_sgpd_tag(AVIOContext *pb, int16_t roll)
>>>>>> +{
>>>>>> + int64_t pos = avio_tell(pb);
>>>>>> + avio_wb32(pb, 0); /* size */
>>>>>> +
>>>>>> + ffio_wfourcc(pb, "sgpd");
>>>>>> + avio_w8(pb, 1); /* version */
>>>>>> + avio_w8(pb, 0); /* flags (1) */
>>>>>> + avio_wb16(pb, 0); /* flags (2) */
>>>>>> + ffio_wfourcc(pb, "roll"); /* grouping type */
>>>>>> + avio_wb32(pb, 2); /* table entry length */
>>>>>> + avio_wb32(pb, 1); /* table entry count */
>>>>>> +
>>>>>> + /* table data, roll distance
>>>>>> + * i.e. number of audio frames to pre-roll after a seek */
>>>>>> + avio_wb16(pb, roll);
>>>>>> +
>>>>>> + return update_size(pb, pos);
>>>>>> +}
>>>>>> +
>>>>>> +static int mov_write_sbgp_tag(AVIOContext *pb, MOVTrack *track)
>>>>>> +{
>>>>>> + int count = 0;
>>>>>> + int i;
>>>>>> + int64_t pos;
>>>>>> +
>>>>>> + for (i = 0; i < track->entry; i++)
>>>>>> + {
>>>>>> + count += track->cluster[i].entries;
>>>>>> + }
>>>>> The starting brace should be on the same line as for, or omitted
>>>> Oops, I know this, but habits are hard to break.
>>>>
>>>>>> +
>>>>>> + pos = avio_tell(pb);
>>>>>> + avio_wb32(pb, 0); /* size */
>>>>>> +
>>>>>> + ffio_wfourcc(pb, "sbgp"); /* atom name */
>>>>>> + avio_wb32(pb, 0); /* version & flags */
>>>>>> + ffio_wfourcc(pb, "roll"); /* grouping type */
>>>>>> + avio_wb32(pb, 1); /* table entry count */
>>>>>> +
>>>>>> + /* table data */
>>>>>> + avio_wb32(pb, count);
>>>>>> + /* sgpd table index, index values are 1 based
>>>>>> + * we write 'roll' sample group at index 1 */
>>>>>> + avio_wb32(pb, 1);
>>>>>> +
>>>>>> + return update_size(pb, pos);
>>>>>> +}
>>>>>> +
>>>>>> /* Sample size atom */
>>>>>> static int mov_write_stsz_tag(AVIOContext *pb, MOVTrack *track)
>>>>>> {
>>>>>> @@ -1260,6 +1308,7 @@ static int mov_write_dref_tag(AVIOContext *pb)
>>>>>>
>>>>>> static int mov_write_stbl_tag(AVFormatContext *s, AVIOContext *pb,
>>>>>> MOVTrack *track)
>>>>>> {
>>>>>> + MOVMuxContext *mov = s->priv_data;
>>>>>> int64_t pos = avio_tell(pb);
>>>>>> avio_wb32(pb, 0); /* size */
>>>>>> ffio_wfourcc(pb, "stbl");
>>>>>> @@ -1277,6 +1326,20 @@ static int mov_write_stbl_tag(AVFormatContext *s,
>>>>>> AVIOContext *pb, MOVTrack *tra
>>>>>> mov_write_stsc_tag(pb, track);
>>>>>> mov_write_stsz_tag(pb, track);
>>>>>> mov_write_stco_tag(pb, track);
>>>>>> +
>>>>>> + if (track->par->codec_id == AV_CODEC_ID_AAC && mov->use_editlist &&
>>>>>> + track->start_dts != AV_NOPTS_VALUE &&
>>>>>> + (track->start_dts < 0 || track->par->initial_padding > 0)) {
>>>>>> + /* Add sgpd and sbgp tags for AAC tracks.
>>>>>> + * Apple documentation says they use this as a flag to indicate
>>>>>> + * that AAC encoder delay is explicitely set in the edit list.
>>>>>> + * If this is omitted, Apple tools will apply an implicit
>>>>>> + * rule to remove encoder delay samples, which results in
>>>>>> + * removal of 2 * delay and A/V desync. */
>>>>>> + mov_write_sgpd_tag(pb, -1);
>>>>> The magic value -1 could probably be described here. From the code in
>>>>> mov_write_sgpd_tag, it seems like it would be a normal actual value, but
>>>>> this seems like a special case that isn't explained elsewhere.
>>>>>
>>>>> Otherwise this seems ok.
>>>>>
>>>>>
>>>> The parameter "roll" is documented where it is used in
>>>> mov_write_sgpd_tag. But I can explain it here as well.
>>> Yes, there I see that it says "number of audio frames to pre-roll after a
>>> seek". But what does -1 frames mean?
>>>
>>>
>> Ah, ok. Would it be clearer like this? "roll is an offset in frames that
>> should be applied when seeking in order to
>> pre-roll data to the decoder"
> I see - then it makes more sense. I guess this means in practice it won't
> ever be positive?
>
>ISO 14496-12 says, "A positive value indicates the number of samples after the sample that is a group member that must be decoded such that at the last of these recovery is complete, i.e. the last sample is correct. A negative value indicates the number of samples before the sample that is a group member that must be decoded in order for recovery to be complete at the marked sample." I should probably just put the above description in the comment since it's quite clear. Apple documentation only talks about the negative variation. And since the objective of this patch is to fix a sync problem with Apple tools, I stuck to what the apple documentation says. I could test it with a positive value to see what the apple tools do in this case if you have a preference for writing this with a positive value. -- 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
