[FFmpeg-devel] [PATCH] libavformat: libavformat: output cues for each subtitle block in MKV muxer

[Michael Niedermayer]

On Sun, May 25, 2014 at 10:05:24AM -0400, John Peebles wrote:
> > This patch doesn't apply on FFmpeg git master.
> The new version attached should apply.
> 
> > please do not include re-indent change into the patch
> done
> 
> > Can you also provide a sample and command line to reproduce?
> Sure.
> (1) Download the attached same-time-script.mkv file. Note that while
> this file only contains a subtitle track in order to keep the file size
> down,
> the bug still happens even if there are other tracks.
> (2) Run ffmpeg -i same-time-script.ass -c copy -map 0 bad.mkv
> (3) Inspect the ebml data in bad.mkv. Notice that while there are two
> subtitle blocks (one for each of two different subtitles), there is only a
> CueTrackPositions element for the first block. There should be one for
> each block.
> 
> > Is the issue you are fixing the same as ticket #3149?
> No, this patch only deals with cues for subtitle tracks, not audio tracks.
> 
> 
> On Sun, May 25, 2014 at 5:20 AM, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:
> 
> > John Peebles <johnpeeb <at> gmail.com> writes:
> >
> > > If you use ffmpeg to remux an mkv file and the file
> > > has ASS subtitles, only the first subtitle with a
> > > given timecode will be added to the list of cues.
> >
> > Is the issue you are fixing the same as ticket #3149?
> > If yes, please add "Fixes ticket #3149" to the
> > commit message.
> >
> > Carl Eugen
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >

>  matroskaenc.c |   71 +++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 39 insertions(+), 32 deletions(-)
> 91aa60e6f0c575c5d4a455c991c236252bc041d7  0001-libavformat-output-cues-for-each-subtitle-block-in-M.patch
> From 722516d9e29ae6be2d65d86ba3a0d7c43771abc7 Mon Sep 17 00:00:00 2001
> From: John Peebles <johnpeeb at gmail.com>
> Date: Thu, 22 May 2014 03:22:15 -0400
> Subject: [PATCH] libavformat: output cues for each subtitle block in MKV muxer
> 
> Signed-off-by: John Peebles <johnpeeb at gmail.com>
> ---
>  libavformat/matroskaenc.c | 71 ++++++++++++++++++++++++++---------------------
>  1 file changed, 39 insertions(+), 32 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 980bf3a..dbb7cae 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -425,8 +425,9 @@ static int mkv_add_cuepoint(mkv_cues *cues, int stream, int tracknum, int64_t ts
>      return 0;
>  }
>  
> -static int64_t mkv_write_cues(AVIOContext *pb, mkv_cues *cues, mkv_track *tracks, int num_tracks)
> +static int64_t mkv_write_cues(AVFormatContext *s, mkv_cues *cues, mkv_track *tracks, int num_tracks)
>  {
> +    AVIOContext *pb = s->pb;
>      ebml_master cues_element;
>      int64_t currentpos;
>      int i, j;
> @@ -449,7 +450,7 @@ static int64_t mkv_write_cues(AVIOContext *pb, mkv_cues *cues, mkv_track *tracks
>          for (j = 0; j < cues->num_entries - i && entry[j].pts == pts; j++) {
>              int tracknum = entry[j].stream_idx;
>              av_assert0(tracknum>=0 && tracknum<num_tracks);
> -            if (tracks[tracknum].has_cue)
> +            if (tracks[tracknum].has_cue && s->streams[tracknum]->codec->codec_type != AVMEDIA_TYPE_SUBTITLE)
>                  continue;
>              tracks[tracknum].has_cue = 1;
>              track_positions = start_ebml_master(pb, MATROSKA_ID_CUETRACKPOSITION, MAX_CUETRACKPOS_SIZE);
> @@ -1278,26 +1279,29 @@ static int ass_get_duration(const uint8_t *p)
>  }
>  
>  #if FF_API_ASS_SSA
> -static int mkv_write_ass_blocks(AVFormatContext *s, AVIOContext *pb, AVPacket *pkt)
> +/* Writes the contents of pkt to a block, using data starting at *data.
> + * If pkt corresponds to more than one block, this writes the contents of the first block
> + * and updates *data so it points to the beginning of the data that corresponds to
> + * the next block.
> + */
> +static int mkv_write_ass_block(AVFormatContext *s, AVIOContext *pb, AVPacket *pkt, uint8_t **data)
>  {
>      MatroskaMuxContext *mkv = s->priv_data;
> -    int i, layer = 0, max_duration = 0, size, line_size, data_size = pkt->size;
> -    uint8_t *start, *end, *data = pkt->data;
> +    int i, layer = 0, size, line_size, data_size = pkt->size - (*data - pkt->data);
> +    uint8_t *start, *end;
>      ebml_master blockgroup;
>      char buffer[2048];
>  
> -    while (data_size) {
> -        int duration = ass_get_duration(data);
> -        max_duration = FFMAX(duration, max_duration);
> -        end = memchr(data, '\n', data_size);
> -        size = line_size = end ? end-data+1 : data_size;
> +        int duration = ass_get_duration(*data);
> +        end = memchr(*data, '\n', data_size);
> +        size = line_size = end ? end-*data+1 : data_size;
>          size -= end ? (end[-1]=='\r')+1 : 0;
> -        start = data;
> +        start = *data;
>          for (i=0; i<3; i++, start++)
> -            if (!(start = memchr(start, ',', size-(start-data))))
> -                return max_duration;
> -        size -= start - data;
> -        sscanf(data, "Dialogue: %d,", &layer);
> +            if (!(start = memchr(start, ',', size-(start-*data))))
> +                return duration;
> +        size -= start - *data;
> +        sscanf(*data, "Dialogue: %d,", &layer);
>          i = snprintf(buffer, sizeof(buffer), "%"PRId64",%d,",
>                       s->streams[pkt->stream_index]->nb_frames, layer);
>          size = FFMIN(i+size, sizeof(buffer));

this diff could be made a bit simpler as well as reducing the risk of
bugs from applying older patches when data is left a uint8_t *
and a  uint8_t ** datap is used as function argument

also, feel free to add yourself to MAINTAINERs for subtitles in
matroska


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140531/bb691c26/attachment.asc>