[FFmpeg-devel] [PATCH] Added queueing for OutputStreams to sidestep DVB subtitle encoding bug.

[Clément Bœsch]

On Fri, Nov 15, 2013 at 05:16:17PM +0100, Wim Vander Schelden wrote:
> Seems like something went wrong when pasting the patch into my email
> client, I've attached it to this mail instead.
> 
> 
> On Fri, Nov 15, 2013 at 4:50 PM, Wim Vander Schelden <wim at fixnum.org> wrote:
> 
> >
> > Hi,
> >
> > I've got a possible solution for http://trac.ffmpeg.org/ticket/2024. It's
> > not the prettiest or most elegant solution, but it "works for my samples
> > "™. I'm open to feedback on how to improve this, I'd love to get this fix
> > merged.
> >
> > Kind regards,
> >
> > Wim Vander Schelden
> > http://fixnum.org
> >
[...]
> From 0902d68182e4c2075bfd3bd0fff5a0d5a25cebbe Mon Sep 17 00:00:00 2001
> From: Wim Vander Schelden <wim at fixnum.org>
> Date: Fri, 15 Nov 2013 16:40:32 +0100
> Subject: [PATCH] Added queueing for OutputStreams to sidestep DVB subtitle
>  encoding bug.
> 
> 
> Signed-off-by: Wim Vander Schelden <wim at fixnum.org>
> ---
>  ffmpeg.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  ffmpeg.h |  9 +++++++++
>  2 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 2a88535..2177784 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -543,6 +543,51 @@ static void update_benchmark(const char *fmt, ...)
>      }
>  }
>  
> +#define STAMP(pkt) ( ((pkt)->dts == AV_NOPTS_VALUE) ? (pkt)->pts : (pkt)->dts)
> +
> +static void write_frame(AVFormatContext *s, AVPacket *pkt, OutputStream *ost);
> +static void queue_frame(AVFormatContext *s, AVPacket *pkt, OutputStream *ost)
> +{
> +    PacketQueue *last = ost->queue, *new;
> +    new = av_mallocz(sizeof(PacketQueue));
> +    new->s   = s;
> +    new->pkt = av_memdup(pkt, sizeof(AVPacket));

This doesn't look safe at all, maybe use av_packet_ref/av_packet_unref.

> +    new->ost = ost;
> +
> +    if(STAMP(pkt) == AV_NOPTS_VALUE) {
> +	write_frame(s, pkt, ost);
> +	return;

Here and a lot below: tabs are not allowed in FFmpeg.

> +    }
> +
> +    if(!last) {
> +	ost->queue = new;
> +	return;
> +    }
> +
> +    while(last && last->next && STAMP(last->next->pkt) < STAMP(pkt)) {
> +	last = last->next;
> +    }
> +
> +    new->next  = last->next;
> +    last->next = new;
> +}
> +
> +static void flush_queue(OutputStream *ost, int64_t ts)
> +{
> +    PacketQueue *last;
> +    
> +    if(ts == AV_NOPTS_VALUE)
> +	return;
> +    
> +    while(ost->queue && STAMP(ost->queue->pkt) <= ts) {
> +	last = ost->queue;
> +	ost->queue = ost->queue->next;
> +	write_frame(last->s, last->pkt, last->ost);
> +	av_free(last->pkt);
> +	av_free(last);
> +    }
> +}
> +

I'd say you should just add one AVPacket *sub_flush_pkt in the context
instead of a whole queue. It would simplify the code a lot.

>  static void write_frame(AVFormatContext *s, AVPacket *pkt, OutputStream *ost)
>  {
>      AVBitStreamFilterContext *bsfc = ost->bitstream_filters;
> @@ -604,6 +649,8 @@ static void write_frame(AVFormatContext *s, AVPacket *pkt, OutputStream *ost)
>          bsfc = bsfc->next;
>      }
>  
> +    flush_queue(ost, STAMP(pkt));
> +
>      if (!(s->oformat->flags & AVFMT_NOTIMESTAMPS) &&
>          (avctx->codec_type == AVMEDIA_TYPE_AUDIO || avctx->codec_type == AVMEDIA_TYPE_VIDEO) &&
>          pkt->dts != AV_NOPTS_VALUE &&
> @@ -651,6 +698,8 @@ static void close_output_stream(OutputStream *ost)
>  {
>      OutputFile *of = output_files[ost->file_index];
>  
> +    flush_queue(ost, INT64_MAX);
> +
>      ost->finished = 1;
>      if (of->shortest) {
>          int64_t end = av_rescale_q(ost->sync_opts - ost->first_pts, ost->st->codec->time_base, AV_TIME_BASE_Q);
> @@ -788,7 +837,10 @@ static void do_subtitle_out(AVFormatContext *s,
>                  pkt.pts += 90 * sub->end_display_time;
>          }
>          subtitle_size += pkt.size;
> -        write_frame(s, &pkt, ost);
> +	if(i == 0)
> +	    write_frame(s, &pkt, ost);
> +	else
> +	    queue_frame(s, &pkt, ost);

OK now, what about making the demuxer cache one packet and fix the
timestamps itself?

[...]

Does it fixes the ticket (the remuxing part at least) or just your use
case?

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131121/44124a1f/attachment.asc>