[FFmpeg-devel] [PATCH 14/15] avformat/matroskaenc: Improve log messages for blocks

[Andreas Rheinhardt]

Michael Niedermayer:
> On Sun, Apr 21, 2019 at 11:04:00PM +0000, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> On Sat, Apr 20, 2019 at 01:41:09AM +0200, Andreas Rheinhardt wrote:
>>>> Up until now, a block's relative offset has been reported as the offset
>>>> in the log messages output when writing blocks; given that it is
>>>> impossible to know the real offset from the beginning of the file at
>>>> this point due to the fact that it is not yet known how many bytes will
>>>> be used for the containing cluster's length field both the relative
>>>> offset in the cluster as well as the offset of the containing cluster
>>>> will be reported from now on.
>>>>
>>>> Also, the log message for writing vtt blocks has been brought in line
>>>> with the message for normal blocks.
>>>>
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>>>> ---
>>>>  libavformat/matroskaenc.c | 15 ++++++++-------
>>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>>> index 441315e2d5..fc0e95440e 100644
>>>> --- a/libavformat/matroskaenc.c
>>>> +++ b/libavformat/matroskaenc.c
>>>> @@ -2124,10 +2124,10 @@ static void mkv_write_block(AVFormatContext *s, AVIOContext *pb,
>>>>  
>>>>      ts += mkv->tracks[pkt->stream_index].ts_offset;
>>>>  
>>>> -    av_log(s, AV_LOG_DEBUG, "Writing block at offset %" PRIu64 ", size %d, "
>>>> -           "pts %" PRId64 ", dts %" PRId64 ", duration %" PRId64 ", keyframe %d\n",
>>>> -           avio_tell(pb), pkt->size, pkt->pts, pkt->dts, pkt->duration,
>>>> -           keyframe != 0);
>>>> +    av_log(s, AV_LOG_DEBUG, "Writing block at relative offset %" PRId64 " in "
>>>> +           "cluster at offset %" PRId64 "; size %d, pts %" PRId64 ", dts %" PRId64
>>>> +           ", duration %" PRId64 ", keyframe %d\n", avio_tell(pb), mkv->cluster_pos,
>>>> +           pkt->size, pkt->pts, pkt->dts, pkt->duration, keyframe != 0);
>>>>      if (par->codec_id == AV_CODEC_ID_H264 && par->extradata_size > 0 &&
>>>>          (AV_RB24(par->extradata) == 1 || AV_RB32(par->extradata) == 1))
>>>>          ff_avc_parse_nal_units_buf(pkt->data, &data, &size);
>>>
>>>> @@ -2231,9 +2231,10 @@ static int mkv_write_vtt_blocks(AVFormatContext *s, AVIOContext *pb, AVPacket *p
>>>>  
>>>>      size = id_size + 1 + settings_size + 1 + pkt->size;
>>>>  
>>>> -    av_log(s, AV_LOG_DEBUG, "Writing block at offset %" PRIu64 ", size %d, "
>>>> -           "pts %" PRId64 ", dts %" PRId64 ", duration %" PRId64 ", flags %d\n",
>>>> -           avio_tell(pb), size, pkt->pts, pkt->dts, pkt->duration, flags);
>>>> +    av_log(s, AV_LOG_DEBUG, "Writing block at relative offset %" PRId64 " in "
>>>> +           "cluster at offset %" PRId64 "; size %d, pts %" PRId64 ", dts %" PRId64
>>>> +           ", duration %" PRId64 ", keyframe %d\n", avio_tell(pb), mkv->cluster_pos,
>>>> +           size, pkt->pts, pkt->dts, pkt->duration, 1);
>>>
>>> It does feel a bit odd to pass a litteral 1 as argument to %d to print a 1.
>>> why is that not a "1" or ommited ?
>>> also these 2 look a bit duplicated
>>> and its a unreadable spagethi, iam sure this can be made more readable with
>>> some line break changes
>>
>> The duplication is intentional: It means that there needs to be only
>> one string literal in the actual binary. Given that the first
> 
> hmm, ok
> it is still duplicated in the source though and this can mutate by being
> edited by someone not knowing it should stay in sync.
> so it may be better to put that string in s static const or #define
> or the printing code itself could be in its own function
> 
Actually, I intend to integrate the writing of WebVTT subtitles into
the main write_block function. This will probably be done once I find
the time to implement support for the Matroska way of WebVTT subtitles
(currently only the WebM way is supported). This uses BlockAdditions
and so it would be natural to do this inside write_block as it already
contains code for writing block additions.
But for the time being, a comment will suffice. (The last change to
this part of the code happened four years ago, so I don't think that
someone will inadvertently unsync them any time soon.)

- Andreas

PS: The Matroska muxer currently doesn't write the MaxBlockAdditionID
element at all, but this is needed for tracks with BlockAdditions,
i.e. our muxer currently creates spec-incompliant files if it uses
BlockAdditions. But that's something for another day, too.