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

[Michael Niedermayer]

On Mon, Apr 22, 2019 at 11:14:00PM +0000, Andreas Rheinhardt wrote:
> 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.)

ok

thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190424/89370fbf/attachment.sig>