[2/3] movenc: Don't write any edit list if the start offset is zero

Message ID 20170219212217.80933-2-martin@martin.st
State Committed
Commit c415c8104c46f5a75306fff1235124b189e86d06
Headers show

Commit Message

Martin Storsjö Feb. 19, 2017, 9:22 p.m.
In these cases, the CTTS flag is set, but no edit list is necessary.
---
 libavformat/movenc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Yusuke Nakamura Feb. 21, 2017, 5:38 p.m. | #1
2017-02-20 6:22 GMT+09:00 Martin Storsjö <martin@martin.st>:

> In these cases, the CTTS flag is set, but no edit list is necessary.
> ---
>  libavformat/movenc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 713c145..f1c2253 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -1811,8 +1811,7 @@ static int mov_write_trak_tag(AVIOContext *pb,
> MOVMuxContext *mov,
>      ffio_wfourcc(pb, "trak");
>      mov_write_tkhd_tag(pb, mov, track, st);
>      if (track->start_dts != AV_NOPTS_VALUE &&
> -        (track->mode == MODE_PSP || track->flags & MOV_TRACK_CTTS ||
> -        track->start_dts || is_clcp_track(track))) {
> +        (track->mode == MODE_PSP || track->start_dts ||
> is_clcp_track(track))) {
>          if (mov->use_editlist)
>              mov_write_edts_tag(pb, mov, track);  // PSP Movies require
> edts box
>          else if ((track->entry && track->cluster[0].dts) || track->mode
> == MODE_PSP || is_clcp_track(track))
> --
> 2.10.1 (Apple Git-78)
>
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel


I dislike this patch since an implicit one‐to‐one mapping of presentation
timeline and media timeline does not specify the relative rate. For
example, QuickTime plays a track at double rate relative to a corresponding
media if the media duration is twice as much as the track duration.
Martin Storsjö Feb. 21, 2017, 9:45 p.m. | #2
Hi Yusuke,

On Wed, 22 Feb 2017, Yusuke Nakamura wrote:

> 2017-02-20 6:22 GMT+09:00 Martin Storsjö <martin@martin.st>:
>
>> In these cases, the CTTS flag is set, but no edit list is necessary.
>> ---
>>  libavformat/movenc.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>> index 713c145..f1c2253 100644
>> --- a/libavformat/movenc.c
>> +++ b/libavformat/movenc.c
>> @@ -1811,8 +1811,7 @@ static int mov_write_trak_tag(AVIOContext *pb,
>> MOVMuxContext *mov,
>>      ffio_wfourcc(pb, "trak");
>>      mov_write_tkhd_tag(pb, mov, track, st);
>>      if (track->start_dts != AV_NOPTS_VALUE &&
>> -        (track->mode == MODE_PSP || track->flags & MOV_TRACK_CTTS ||
>> -        track->start_dts || is_clcp_track(track))) {
>> +        (track->mode == MODE_PSP || track->start_dts ||
>> is_clcp_track(track))) {
>>          if (mov->use_editlist)
>>              mov_write_edts_tag(pb, mov, track);  // PSP Movies require
>> edts box
>>          else if ((track->entry && track->cluster[0].dts) || track->mode
>> == MODE_PSP || is_clcp_track(track))
>> --
>> 2.10.1 (Apple Git-78)
>>
>> _______________________________________________
>> libav-devel mailing list
>> libav-devel@libav.org
>> https://lists.libav.org/mailman/listinfo/libav-devel
>
>
> I dislike this patch since an implicit one‐to‐one mapping of presentation
> timeline and media timeline does not specify the relative rate. For
> example, QuickTime plays a track at double rate relative to a corresponding
> media if the media duration is twice as much as the track duration.

I don't really follow in which concrete case this patch would make a 
difference? With patch 1/3 applied, if one enables the 
negative_cts_offsets flag that I added, one will end up with an 
unnecessary edit list. As far as I can see right now, this is the only 
case where this patch (currently) would make any difference.

Is there any case where it's beneficial to write out an edit list even 
though it's with media_time = 0, and with the full duration of the track?

// Martin
Yusuke Nakamura Feb. 22, 2017, 11:44 p.m. | #3
2017-02-22 6:45 GMT+09:00 Martin Storsjö <martin@martin.st>:

> Hi Yusuke,
>
>
> On Wed, 22 Feb 2017, Yusuke Nakamura wrote:
>
> 2017-02-20 6:22 GMT+09:00 Martin Storsjö <martin@martin.st>:
>>
>> In these cases, the CTTS flag is set, but no edit list is necessary.
>>> ---
>>>  libavformat/movenc.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>> index 713c145..f1c2253 100644
>>> --- a/libavformat/movenc.c
>>> +++ b/libavformat/movenc.c
>>> @@ -1811,8 +1811,7 @@ static int mov_write_trak_tag(AVIOContext *pb,
>>> MOVMuxContext *mov,
>>>      ffio_wfourcc(pb, "trak");
>>>      mov_write_tkhd_tag(pb, mov, track, st);
>>>      if (track->start_dts != AV_NOPTS_VALUE &&
>>> -        (track->mode == MODE_PSP || track->flags & MOV_TRACK_CTTS ||
>>> -        track->start_dts || is_clcp_track(track))) {
>>> +        (track->mode == MODE_PSP || track->start_dts ||
>>> is_clcp_track(track))) {
>>>          if (mov->use_editlist)
>>>              mov_write_edts_tag(pb, mov, track);  // PSP Movies require
>>> edts box
>>>          else if ((track->entry && track->cluster[0].dts) || track->mode
>>> == MODE_PSP || is_clcp_track(track))
>>> --
>>> 2.10.1 (Apple Git-78)
>>>
>>> _______________________________________________
>>> libav-devel mailing list
>>> libav-devel@libav.org
>>> https://lists.libav.org/mailman/listinfo/libav-devel
>>>
>>
>>
>> I dislike this patch since an implicit one‐to‐one mapping of presentation
>> timeline and media timeline does not specify the relative rate. For
>> example, QuickTime plays a track at double rate relative to a
>> corresponding
>> media if the media duration is twice as much as the track duration.
>>
>
> I don't really follow in which concrete case this patch would make a
> difference? With patch 1/3 applied, if one enables the negative_cts_offsets
> flag that I added, one will end up with an unnecessary edit list. As far as
> I can see right now, this is the only case where this patch (currently)
> would make any difference.
>
> Is there any case where it's beneficial to write out an edit list even
> though it's with media_time = 0, and with the full duration of the track?
>

I see. this patch makes no change in existing implementation.

This is a good time to tell about my stance.
I think, basically, edit lists should be write as much as possible to
indicate writer's intention. At least within descriptions of the current
isobmff spec, demuxers might interpret implicit edits as they like. (i
realized that it's too difficult to understand how edits work even if edits
are explicit things while reading drafts and defect reports flowing on
mp4-sys ml. apparently edit list is veiled in mystery even for mpeg
members). The spec does not explicitly say a track presentation starts from
composition time 0, only says that all tracks start from presentation time
0 when edit list is absent, and the spec does not specify rule to determine
the relative rate,thus, those behavior cannot be predicted on wild demuxers.

I'm sorry for making a noise.


> // Martin
>
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
>

Patch

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 713c145..f1c2253 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -1811,8 +1811,7 @@  static int mov_write_trak_tag(AVIOContext *pb, MOVMuxContext *mov,
     ffio_wfourcc(pb, "trak");
     mov_write_tkhd_tag(pb, mov, track, st);
     if (track->start_dts != AV_NOPTS_VALUE &&
-        (track->mode == MODE_PSP || track->flags & MOV_TRACK_CTTS ||
-        track->start_dts || is_clcp_track(track))) {
+        (track->mode == MODE_PSP || track->start_dts || is_clcp_track(track))) {
         if (mov->use_editlist)
             mov_write_edts_tag(pb, mov, track);  // PSP Movies require edts box
         else if ((track->entry && track->cluster[0].dts) || track->mode == MODE_PSP || is_clcp_track(track))