matroskaenc: Fix writing zero duration subtitles

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

Commit Message

John Stebbins Oct. 29, 2014, 6:06 p.m.
The matroska spec says blockduration == 0 means the frame is not a
keyframe.  Since all subtitles are "keyframes", 0 blockduration should
not be written.

Fixes mkvalidator error messages for PGS subtitles.
---
 libavformat/matroskaenc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Luca Barbato Oct. 29, 2014, 6:29 p.m. | #1
On 29/10/14 19:06, John Stebbins wrote:
> The matroska spec says blockduration == 0 means the frame is not a
> keyframe.  Since all subtitles are "keyframes", 0 blockduration should
> not be written.
>
> Fixes mkvalidator error messages for PGS subtitles.
> ---
>   libavformat/matroskaenc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>

Ok.
Vittorio Giovara Oct. 29, 2014, 6:44 p.m. | #2
On Wed, Oct 29, 2014 at 6:06 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
> The matroska spec says blockduration == 0 means the frame is not a
> keyframe.  Since all subtitles are "keyframes", 0 blockduration should
> not be written.
>
> Fixes mkvalidator error messages for PGS subtitles.
> ---
>  libavformat/matroskaenc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 4ec474d..628300b 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1487,7 +1487,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 (duration > 0)
> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>          end_ebml_master(pb, blockgroup);

Should we just skip or report an error?
Could you double check the mkv_write_ass_blocks() where a similar thing happens?
Thanks
John Stebbins Oct. 29, 2014, 6:51 p.m. | #3
On 10/29/2014 11:44 AM, Vittorio Giovara wrote:
> On Wed, Oct 29, 2014 at 6:06 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
>> The matroska spec says blockduration == 0 means the frame is not a
>> keyframe.  Since all subtitles are "keyframes", 0 blockduration should
>> not be written.
>>
>> Fixes mkvalidator error messages for PGS subtitles.
>> ---
>>  libavformat/matroskaenc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index 4ec474d..628300b 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -1487,7 +1487,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 (duration > 0)
>> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>>          end_ebml_master(pb, blockgroup);
> Should we just skip or report an error?
> Could you double check the mkv_write_ass_blocks() where a similar thing happens?
> Thanks

In the case I am fixing, it is valid to have 0 duration, it's not an error.  PGS subtitles don't require a duration
because there is an explicit "empty" subtitle that marks the end of every subtitle that has content.  The timestamps of
these empty subtitles define the end of the previous subtitle.

In the case of ass and srt subtitles it may be a good idea to report an error since these types of subtitles should
never be 0 duration.
Tim Walker Oct. 29, 2014, 6:54 p.m. | #4
On 29 Oct 2014, at 19:06, John Stebbins <stebbins@jetheaddev.com> wrote:

> The matroska spec says blockduration == 0 means the frame is not a
> keyframe.  Since all subtitles are "keyframes", 0 blockduration should
> not be written.
> 
> Fixes mkvalidator error messages for PGS subtitles.
> ---
> libavformat/matroskaenc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)

Please CC libav-stable, this bugfix is worth backporting IMO.

Tim
Vittorio Giovara Oct. 29, 2014, 7:39 p.m. | #5
On Wed, Oct 29, 2014 at 6:51 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
> On 10/29/2014 11:44 AM, Vittorio Giovara wrote:
>> On Wed, Oct 29, 2014 at 6:06 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
>>> The matroska spec says blockduration == 0 means the frame is not a
>>> keyframe.  Since all subtitles are "keyframes", 0 blockduration should
>>> not be written.
>>>
>>> Fixes mkvalidator error messages for PGS subtitles.
>>> ---
>>>  libavformat/matroskaenc.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>> index 4ec474d..628300b 100644
>>> --- a/libavformat/matroskaenc.c
>>> +++ b/libavformat/matroskaenc.c
>>> @@ -1487,7 +1487,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 (duration > 0)
>>> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>>>          end_ebml_master(pb, blockgroup);
>> Should we just skip or report an error?
>> Could you double check the mkv_write_ass_blocks() where a similar thing happens?
>> Thanks
>
> In the case I am fixing, it is valid to have 0 duration, it's not an error.  PGS subtitles don't require a duration
> because there is an explicit "empty" subtitle that marks the end of every subtitle that has content.  The timestamps of
> these empty subtitles define the end of the previous subtitle.

Thanks for clarifying.

> In the case of ass and srt subtitles it may be a good idea to report an error since these types of subtitles should
> never be 0 duration.

Can I ask for a patchy patch? :)
Feel free to amend this one if you prefer.

Also yes, CC: libav-stable@libav.org
wm4 Oct. 29, 2014, 10:30 p.m. | #6
On Wed, 29 Oct 2014 11:06:05 -0700
John Stebbins <stebbins@jetheaddev.com> wrote:

> The matroska spec says blockduration == 0 means the frame is not a
> keyframe.  Since all subtitles are "keyframes", 0 blockduration should
> not be written.
> 
> Fixes mkvalidator error messages for PGS subtitles.
> ---
>  libavformat/matroskaenc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 4ec474d..628300b 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1487,7 +1487,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 (duration > 0)
> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>          end_ebml_master(pb, blockgroup);
>      }
>  

This way you can't distinguish subtitles with duration 0, and subtitles
with unknown duration.

Even mkvmerge writes ASS subtitles with duration 0 with an explicit
blockduration element:

| + Block group at 6289
|  + Block (track number 1, 1 frame(s), timecode 60.000s = 00:01:00.000) at 6291
|   + Frame with size 21
|  + Block duration: 0.000000ms at 6318
| + [I frame for track 1, timecode 60000]

Don't apply this patch, and burn your copy of the Matroska "spec".
Luca Barbato Oct. 29, 2014, 10:40 p.m. | #7
On 29/10/14 23:30, wm4 wrote:
> Even mkvmerge writes ASS subtitles with duration 0 with an explicit
> blockduration element:
>
> | + Block group at 6289
> |  + Block (track number 1, 1 frame(s), timecode 60.000s = 00:01:00.000) at 6291
> |   + Frame with size 21
> |  + Block duration: 0.000000ms at 6318
> | + [I frame for track 1, timecode 60000]
>
> Don't apply this patch, and burn your copy of the Matroska "spec".

Shall we ask the mkv people to amend the spec?

lu
wm4 Oct. 29, 2014, 10:49 p.m. | #8
On Wed, 29 Oct 2014 23:40:21 +0100
Luca Barbato <lu_zero@gentoo.org> wrote:

> On 29/10/14 23:30, wm4 wrote:
> > Even mkvmerge writes ASS subtitles with duration 0 with an explicit
> > blockduration element:
> >
> > | + Block group at 6289
> > |  + Block (track number 1, 1 frame(s), timecode 60.000s = 00:01:00.000) at 6291
> > |   + Frame with size 21
> > |  + Block duration: 0.000000ms at 6318
> > | + [I frame for track 1, timecode 60000]
> >
> > Don't apply this patch, and burn your copy of the Matroska "spec".
> 
> Shall we ask the mkv people to amend the spec?

You can try, I guess: matroska-devel@lists.matroska.org
John Stebbins Oct. 30, 2014, 1:34 a.m. | #9
On 10/29/2014 03:40 PM, Luca Barbato wrote:
> On 29/10/14 23:30, wm4 wrote:
>> Even mkvmerge writes ASS subtitles with duration 0 with an explicit
>> blockduration element:
>>
>> | + Block group at 6289
>> |  + Block (track number 1, 1 frame(s), timecode 60.000s = 00:01:00.000) at 6291
>> |   + Frame with size 21
>> |  + Block duration: 0.000000ms at 6318
>> | + [I frame for track 1, timecode 60000]
>>
>> Don't apply this patch, and burn your copy of the Matroska "spec".
> Shall we ask the mkv people to amend the spec?
>
>

... And fix mkvalidator.  Users checking their files with it think the files are corrupt because this condition is
flagged as a error.
John Stebbins Oct. 30, 2014, 1:40 a.m. | #10
On 10/29/2014 03:30 PM, wm4 wrote:
> On Wed, 29 Oct 2014 11:06:05 -0700
> John Stebbins <stebbins@jetheaddev.com> wrote:
>
>> The matroska spec says blockduration == 0 means the frame is not a
>> keyframe.  Since all subtitles are "keyframes", 0 blockduration should
>> not be written.
>>
>> Fixes mkvalidator error messages for PGS subtitles.
>> ---
>>  libavformat/matroskaenc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index 4ec474d..628300b 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -1487,7 +1487,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 (duration > 0)
>> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>>          end_ebml_master(pb, blockgroup);
>>      }
>>  
> This way you can't distinguish subtitles with duration 0, and subtitles
> with unknown duration.
>
> Even mkvmerge writes ASS subtitles with duration 0 with an explicit
> blockduration element:
>
> | + Block group at 6289
> |  + Block (track number 1, 1 frame(s), timecode 60.000s = 00:01:00.000) at 6291
> |   + Frame with size 21
> |  + Block duration: 0.000000ms at 6318
> | + [I frame for track 1, timecode 60000]
>
> Don't apply this patch, and burn your copy of the Matroska "spec".
>

How do we represent subtitles with unknown duration in libav.  This is really what is needed for PGS anyway.  Should we
be checking for AV_NOPTS_VALUE in convergence_duration instead?
wm4 Oct. 30, 2014, 8:21 p.m. | #11
On Wed, 29 Oct 2014 18:40:27 -0700
John Stebbins <stebbins@jetheaddev.com> wrote:

> On 10/29/2014 03:30 PM, wm4 wrote:
> > On Wed, 29 Oct 2014 11:06:05 -0700
> > John Stebbins <stebbins@jetheaddev.com> wrote:
> >
> >> The matroska spec says blockduration == 0 means the frame is not a
> >> keyframe.  Since all subtitles are "keyframes", 0 blockduration should
> >> not be written.
> >>
> >> Fixes mkvalidator error messages for PGS subtitles.
> >> ---
> >>  libavformat/matroskaenc.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> >> index 4ec474d..628300b 100644
> >> --- a/libavformat/matroskaenc.c
> >> +++ b/libavformat/matroskaenc.c
> >> @@ -1487,7 +1487,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 (duration > 0)
> >> +            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
> >>          end_ebml_master(pb, blockgroup);
> >>      }
> >>  
> > This way you can't distinguish subtitles with duration 0, and subtitles
> > with unknown duration.
> >
> > Even mkvmerge writes ASS subtitles with duration 0 with an explicit
> > blockduration element:
> >
> > | + Block group at 6289
> > |  + Block (track number 1, 1 frame(s), timecode 60.000s = 00:01:00.000) at 6291
> > |   + Frame with size 21
> > |  + Block duration: 0.000000ms at 6318
> > | + [I frame for track 1, timecode 60000]
> >
> > Don't apply this patch, and burn your copy of the Matroska "spec".
> >
> 
> How do we represent subtitles with unknown duration in libav.  This is really what is needed for PGS anyway.  Should we
> be checking for AV_NOPTS_VALUE in convergence_duration instead?
> 

No. As far as I know, there's no way yet. Maybe what you propose or
setting the duration to -1 would work.

I think distinguishing these would be good. Some formats have an
implicit duration (the subtitle is removed with a "deletion" packet),
while other formats can have 0 duration and require the renderer to
never actually display such subtitles.

Patch

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 4ec474d..628300b 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1487,7 +1487,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 (duration > 0)
+            put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
         end_ebml_master(pb, blockgroup);
     }