On Sat, 28 Mar 2015 10:04:14 -0600 John Stebbins <[email protected]> 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 <[email protected]> wrote: > >> > >>> On 03/21/2015 06:30 AM, wm4 wrote: > >>>> On Fri, 20 Mar 2015 16:47:57 -0600 > >>>> John Stebbins <[email protected]> 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. _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
