matroskaenc: Don't write AV_NOPTS_VALUE subtitle duration

Message ID 1426891677-18551-1-git-send-email-stebbins@jetheaddev.com
State New
Headers show

Commit Message

John Stebbins March 20, 2015, 10:47 p.m.
Some subtitles (e.g. PGS) do not require a duration since they have
explicit end-of-subtitle indication in the stream.  This provides
a way to omit the unnecessary duration while muxing.
---
 libavformat/matroskaenc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Luca Barbato March 20, 2015, 11:12 p.m. | #1
On 20/03/15 23:47, John Stebbins wrote:
> Some subtitles (e.g. PGS) do not require a duration since they have
> explicit end-of-subtitle indication in the stream.  This provides
> a way to omit the unnecessary duration while muxing.
> ---
>   libavformat/matroskaenc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 49e5bf3..1dd8d74 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1524,7 +1524,8 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
>                                                      mkv_blockgroup_size(pkt->size));
>           duration = pkt->convergence_duration;
>           mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 0);
> -        put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
> +        if (pkt->convergence_duration != AV_NOPTS_VALUE)
> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>           end_ebml_master(pb, blockgroup);
>       }
>
>
Fine for me, will be pushed tomorrow.
wm4 March 21, 2015, 12:30 p.m. | #2
On Fri, 20 Mar 2015 16:47:57 -0600
John Stebbins <stebbins@jetheaddev.com> wrote:

> Some subtitles (e.g. PGS) do not require a duration since they have
> explicit end-of-subtitle indication in the stream.  This provides
> a way to omit the unnecessary duration while muxing.
> ---
>  libavformat/matroskaenc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 49e5bf3..1dd8d74 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1524,7 +1524,8 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
>                                                     mkv_blockgroup_size(pkt->size));
>          duration = pkt->convergence_duration;
>          mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 0);
> -        put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
> +        if (pkt->convergence_duration != AV_NOPTS_VALUE)
> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>          end_ebml_master(pb, blockgroup);
>      }
>  

It's weird that it checks convergence_duration when writing duration. I
also wasn't aware that convergence_duration can be set to AV_NOPTS_VALUE
to signal that it's unset. Isn't 0 the value for unset duration? (The
API can't distinguish between 0 duration and unset duration, this is a
known problem.)
John Stebbins March 21, 2015, 3:20 p.m. | #3
On 03/21/2015 06:30 AM, wm4 wrote:
> On Fri, 20 Mar 2015 16:47:57 -0600
> John Stebbins <stebbins@jetheaddev.com> wrote:
>
>> Some subtitles (e.g. PGS) do not require a duration since they have
>> explicit end-of-subtitle indication in the stream.  This provides
>> a way to omit the unnecessary duration while muxing.
>> ---
>>  libavformat/matroskaenc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index 49e5bf3..1dd8d74 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -1524,7 +1524,8 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
>>                                                     mkv_blockgroup_size(pkt->size));
>>          duration = pkt->convergence_duration;
>>          mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 0);
>> -        put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>> +        if (pkt->convergence_duration != AV_NOPTS_VALUE)
>> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>>          end_ebml_master(pb, blockgroup);
>>      }
>>  
> It's weird that it checks convergence_duration when writing duration. I
> also wasn't aware that convergence_duration can be set to AV_NOPTS_VALUE
> to signal that it's unset. Isn't 0 the value for unset duration? (The
> API can't distinguish between 0 duration and unset duration, this is a
> known problem.)
>

If you recall, a while back I proposed a patch that used 0 for unset.  I'm just (finally) following up on that thread. 
Your response was this:
https://lists.libav.org/pipermail/libav-devel/2014-October/064418.html

You didn't think using 0 for this was a good idea because even mkvmerge wrote 0 for block duration despite the
discrepancy with the "matroska spec".  As for AV_NOPTS_VALUE and convergence_duration, avcodec.h says "Is AV_NOPTS_VALUE
if unknown"
wm4 March 21, 2015, 3:45 p.m. | #4
On Sat, 21 Mar 2015 09:20:40 -0600
John Stebbins <stebbins@jetheaddev.com> wrote:

> 
> On 03/21/2015 06:30 AM, wm4 wrote:
> > On Fri, 20 Mar 2015 16:47:57 -0600
> > John Stebbins <stebbins@jetheaddev.com> wrote:
> >
> >> Some subtitles (e.g. PGS) do not require a duration since they have
> >> explicit end-of-subtitle indication in the stream.  This provides
> >> a way to omit the unnecessary duration while muxing.
> >> ---
> >>  libavformat/matroskaenc.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> >> index 49e5bf3..1dd8d74 100644
> >> --- a/libavformat/matroskaenc.c
> >> +++ b/libavformat/matroskaenc.c
> >> @@ -1524,7 +1524,8 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
> >>                                                     mkv_blockgroup_size(pkt->size));
> >>          duration = pkt->convergence_duration;
> >>          mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 0);
> >> -        put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
> >> +        if (pkt->convergence_duration != AV_NOPTS_VALUE)
> >> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
> >>          end_ebml_master(pb, blockgroup);
> >>      }
> >>  
> > It's weird that it checks convergence_duration when writing duration. I
> > also wasn't aware that convergence_duration can be set to AV_NOPTS_VALUE
> > to signal that it's unset. Isn't 0 the value for unset duration? (The
> > API can't distinguish between 0 duration and unset duration, this is a
> > known problem.)
> >
> 
> If you recall, a while back I proposed a patch that used 0 for unset.  I'm just (finally) following up on that thread. 
> Your response was this:
> https://lists.libav.org/pipermail/libav-devel/2014-October/064418.html
> 
> You didn't think using 0 for this was a good idea because even mkvmerge wrote 0 for block duration despite the
> discrepancy with the "matroska spec".  As for AV_NOPTS_VALUE and convergence_duration, avcodec.h says "Is AV_NOPTS_VALUE
> if unknown"
> 
> 

In that older post I meant that 0 should not be written to the
file. If convergence_duration==AV_NOPTS_VALUE is used consistently it's
fine, although avpacket.c still initializes it to 0 by default.

(Also, isn't it time to break ABI and unify duration and
convergence_duration?)
John Stebbins March 21, 2015, 4:28 p.m. | #5
On 03/21/2015 09:45 AM, wm4 wrote:
> On Sat, 21 Mar 2015 09:20:40 -0600
> John Stebbins <stebbins@jetheaddev.com> wrote:
>
>> On 03/21/2015 06:30 AM, wm4 wrote:
>>> On Fri, 20 Mar 2015 16:47:57 -0600
>>> John Stebbins <stebbins@jetheaddev.com> wrote:
>>>
>>>> Some subtitles (e.g. PGS) do not require a duration since they have
>>>> explicit end-of-subtitle indication in the stream.  This provides
>>>> a way to omit the unnecessary duration while muxing.
>>>> ---
>>>>  libavformat/matroskaenc.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>>> index 49e5bf3..1dd8d74 100644
>>>> --- a/libavformat/matroskaenc.c
>>>> +++ b/libavformat/matroskaenc.c
>>>> @@ -1524,7 +1524,8 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
>>>>                                                     mkv_blockgroup_size(pkt->size));
>>>>          duration = pkt->convergence_duration;
>>>>          mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 0);
>>>> -        put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>>>> +        if (pkt->convergence_duration != AV_NOPTS_VALUE)
>>>> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>>>>          end_ebml_master(pb, blockgroup);
>>>>      }
>>>>  
>>> It's weird that it checks convergence_duration when writing duration. I
>>> also wasn't aware that convergence_duration can be set to AV_NOPTS_VALUE
>>> to signal that it's unset. Isn't 0 the value for unset duration? (The
>>> API can't distinguish between 0 duration and unset duration, this is a
>>> known problem.)
>>>
>> If you recall, a while back I proposed a patch that used 0 for unset.  I'm just (finally) following up on that thread. 
>> Your response was this:
>> https://lists.libav.org/pipermail/libav-devel/2014-October/064418.html
>>
>> You didn't think using 0 for this was a good idea because even mkvmerge wrote 0 for block duration despite the
>> discrepancy with the "matroska spec".  As for AV_NOPTS_VALUE and convergence_duration, avcodec.h says "Is AV_NOPTS_VALUE
>> if unknown"
>>
>>
> In that older post I meant that 0 should not be written to the
> file. If convergence_duration==AV_NOPTS_VALUE is used consistently it's
> fine, although avpacket.c still initializes it to 0 by default.
>
> (Also, isn't it time to break ABI and unify duration and
> convergence_duration?)
>

Using either 0 or AV_NOPTS_VALUE appears to be safe.  Either is fine by me.  So if there is a preference, speak up.
A quick grep shows that convergence_duration is only used in matroskaenc and matroskadec.  It wouldn't be
hard to drop it in favor of duration.  But that's a change you wouldn't want backported to v11 and I would
like this change in behavior to get into v11 if possible.
John Stebbins March 28, 2015, 4:04 p.m. | #6
On 03/21/2015 10:28 AM, John Stebbins wrote:
> On 03/21/2015 09:45 AM, wm4 wrote:
>> On Sat, 21 Mar 2015 09:20:40 -0600
>> John Stebbins <stebbins@jetheaddev.com> wrote:
>>
>>> On 03/21/2015 06:30 AM, wm4 wrote:
>>>> On Fri, 20 Mar 2015 16:47:57 -0600
>>>> John Stebbins <stebbins@jetheaddev.com> wrote:
>>>>
>>>>> Some subtitles (e.g. PGS) do not require a duration since they have
>>>>> explicit end-of-subtitle indication in the stream.  This provides
>>>>> a way to omit the unnecessary duration while muxing.
>>>>> ---
>>>>>  libavformat/matroskaenc.c | 3 ++-
>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>>>> index 49e5bf3..1dd8d74 100644
>>>>> --- a/libavformat/matroskaenc.c
>>>>> +++ b/libavformat/matroskaenc.c
>>>>> @@ -1524,7 +1524,8 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
>>>>>                                                     mkv_blockgroup_size(pkt->size));
>>>>>          duration = pkt->convergence_duration;
>>>>>          mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 0);
>>>>> -        put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>>>>> +        if (pkt->convergence_duration != AV_NOPTS_VALUE)
>>>>> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>>>>>          end_ebml_master(pb, blockgroup);
>>>>>      }
>>>>>  
>>>> It's weird that it checks convergence_duration when writing duration. I
>>>> also wasn't aware that convergence_duration can be set to AV_NOPTS_VALUE
>>>> to signal that it's unset. Isn't 0 the value for unset duration? (The
>>>> API can't distinguish between 0 duration and unset duration, this is a
>>>> known problem.)
>>>>
>>> If you recall, a while back I proposed a patch that used 0 for unset.  I'm just (finally) following up on that thread. 
>>> Your response was this:
>>> https://lists.libav.org/pipermail/libav-devel/2014-October/064418.html
>>>
>>> You didn't think using 0 for this was a good idea because even mkvmerge wrote 0 for block duration despite the
>>> discrepancy with the "matroska spec".  As for AV_NOPTS_VALUE and convergence_duration, avcodec.h says "Is AV_NOPTS_VALUE
>>> if unknown"
>>>
>>>
>> In that older post I meant that 0 should not be written to the
>> file. If convergence_duration==AV_NOPTS_VALUE is used consistently it's
>> fine, although avpacket.c still initializes it to 0 by default.
>>
>> (Also, isn't it time to break ABI and unify duration and
>> convergence_duration?)
>>
> Using either 0 or AV_NOPTS_VALUE appears to be safe.  Either is fine by me.  So if there is a preference, speak up.
> A quick grep shows that convergence_duration is only used in matroskaenc and matroskadec.  It wouldn't be
> hard to drop it in favor of duration.  But that's a change you wouldn't want backported to v11 and I would
> like this change in behavior to get into v11 if possible.
>

No additional opinions?  IMO each option has it's own advantages.  Using AV_NOPTS_VALUE maintains current functionality
when convergence_duration == 0 which may prevent breakage in some scenario that expects this behavior.  On the other
hand, preventing 0 BLOCKDURATION adheres to the letter of the matroska "spec".   I prefer the latter (which was my
original patch), so if there are no objections, I will re-submit that patch given that I seem to have misinterpreted
wm4's comments regarding it.
wm4 March 28, 2015, 8:45 p.m. | #7
On Sat, 28 Mar 2015 10:04:14 -0600
John Stebbins <stebbins@jetheaddev.com> wrote:

> On 03/21/2015 10:28 AM, John Stebbins wrote:
> > On 03/21/2015 09:45 AM, wm4 wrote:
> >> On Sat, 21 Mar 2015 09:20:40 -0600
> >> John Stebbins <stebbins@jetheaddev.com> wrote:
> >>
> >>> On 03/21/2015 06:30 AM, wm4 wrote:
> >>>> On Fri, 20 Mar 2015 16:47:57 -0600
> >>>> John Stebbins <stebbins@jetheaddev.com> wrote:
> >>>>
> >>>>> Some subtitles (e.g. PGS) do not require a duration since they have
> >>>>> explicit end-of-subtitle indication in the stream.  This provides
> >>>>> a way to omit the unnecessary duration while muxing.
> >>>>> ---
> >>>>>  libavformat/matroskaenc.c | 3 ++-
> >>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> >>>>> index 49e5bf3..1dd8d74 100644
> >>>>> --- a/libavformat/matroskaenc.c
> >>>>> +++ b/libavformat/matroskaenc.c
> >>>>> @@ -1524,7 +1524,8 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
> >>>>>                                                     mkv_blockgroup_size(pkt->size));
> >>>>>          duration = pkt->convergence_duration;
> >>>>>          mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 0);
> >>>>> -        put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
> >>>>> +        if (pkt->convergence_duration != AV_NOPTS_VALUE)
> >>>>> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
> >>>>>          end_ebml_master(pb, blockgroup);
> >>>>>      }
> >>>>>  
> >>>> It's weird that it checks convergence_duration when writing duration. I
> >>>> also wasn't aware that convergence_duration can be set to AV_NOPTS_VALUE
> >>>> to signal that it's unset. Isn't 0 the value for unset duration? (The
> >>>> API can't distinguish between 0 duration and unset duration, this is a
> >>>> known problem.)
> >>>>
> >>> If you recall, a while back I proposed a patch that used 0 for unset.  I'm just (finally) following up on that thread. 
> >>> Your response was this:
> >>> https://lists.libav.org/pipermail/libav-devel/2014-October/064418.html
> >>>
> >>> You didn't think using 0 for this was a good idea because even mkvmerge wrote 0 for block duration despite the
> >>> discrepancy with the "matroska spec".  As for AV_NOPTS_VALUE and convergence_duration, avcodec.h says "Is AV_NOPTS_VALUE
> >>> if unknown"
> >>>
> >>>
> >> In that older post I meant that 0 should not be written to the
> >> file. If convergence_duration==AV_NOPTS_VALUE is used consistently it's
> >> fine, although avpacket.c still initializes it to 0 by default.
> >>
> >> (Also, isn't it time to break ABI and unify duration and
> >> convergence_duration?)
> >>
> > Using either 0 or AV_NOPTS_VALUE appears to be safe.  Either is fine by me.  So if there is a preference, speak up.
> > A quick grep shows that convergence_duration is only used in matroskaenc and matroskadec.  It wouldn't be
> > hard to drop it in favor of duration.  But that's a change you wouldn't want backported to v11 and I would
> > like this change in behavior to get into v11 if possible.
> >
> 
> No additional opinions?  IMO each option has it's own advantages.  Using AV_NOPTS_VALUE maintains current functionality
> when convergence_duration == 0 which may prevent breakage in some scenario that expects this behavior.  On the other
> hand, preventing 0 BLOCKDURATION adheres to the letter of the matroska "spec".   I prefer the latter (which was my
> original patch), so if there are no objections, I will re-submit that patch given that I seem to have misinterpreted
> wm4's comments regarding it.
> 

I'm not so sure either what's the best... for srt and ass, if the
duration is 0, 0 should be written. For PGS and Vobsubs no duration
should be written, since these subtitle codecs do not have a concept of
duration on the packet level. If I got this right.

Now the question is whether API users follow this consistently, or
whether matroskaenc.c makes it too easy to write (technically) invalid
files.

Of course AV_NOPTS_VALUE should never be directly written to a mkv
file, so your patch is an improvement in any case.

Patch

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 49e5bf3..1dd8d74 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1524,7 +1524,8 @@  static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
                                                    mkv_blockgroup_size(pkt->size));
         duration = pkt->convergence_duration;
         mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 0);
-        put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
+        if (pkt->convergence_duration != AV_NOPTS_VALUE)
+            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
         end_ebml_master(pb, blockgroup);
     }