[FFmpeg-devel] [PATCH 2/3] lavf/srtenc: do not print more line breaks than necessary.

[Clément Bœsch]

On Thu, Oct 25, 2012 at 03:00:37PM +0200, Nicolas George wrote:
> Le decadi 30 vendémiaire, an CCXXI, Clément Bœsch a écrit :
> > ---
> >  libavformat/srtenc.c         | 12 ++++++++++--
> >  tests/ref/fate/sub-subripenc |  2 +-
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavformat/srtenc.c b/libavformat/srtenc.c
> > index 42a264e..d92f559 100644
> > --- a/libavformat/srtenc.c
> > +++ b/libavformat/srtenc.c
> > @@ -78,8 +78,16 @@ static int srt_write_packet(AVFormatContext *avf, AVPacket *pkt)
> >                         (int)(e /    1000) % 60, (int)(e %  1000));
> >      }
> >      avio_write(avf->pb, pkt->data, pkt->size);
> > -    if (write_ts)
> > -        avio_write(avf->pb, "\n\n", 2);
> > +    if (write_ts) {
> > +        int i, linebreaks = 2;
> > +        for (i = sizeof("\r\n\r\n") - 1; i > 0; i--) {
> > +            char c = pkt->data[pkt->size - i];
> > +            if (pkt->size >= i && c == '\r' || c == '\n')
> > +                linebreaks -= c == '\n';
> > +        }
> > +        for (i = 0; i < linebreaks; i++)
> > +            avio_write(avf->pb, "\n", 1);
> > +    }
> 
> I have two problems with this patch:
> 
> The first one is that I find the logic very hard to follow. For example I am
> not sure there is not a possible array underflow when accessing the data:
> the test i >= pkt->size is done after accessing data[pkt->size - i].
> 

I can rewrite in a more obvious way if needed.

> The second is that I believe this is not the muxer's task to do that. Why
> are these line breaks present in the packet for in the first place? They
> should not be there.
> 

Well, the thing is, originally SRT packets contained everything, so \n\n
or \r\n at the end of each event (except maybe the last one). Now we are
kind of assuming that a SubRip packet will just contain the markup line,
but every demuxer will likely behave differently: some events might ends
with one \r\n, some none at all.

The first case seems legit in case of a SubRip encoder, where a line is
just supposed to ends with a '\n' (arbitrary decision). Our encoder is
actually adding two of them because of the SRT past, where the encoder was
kind of responsible for the muxing (muxing was done with a raw muxer).

The second case could be a muxed SubRip in a random container, where \n
are trimmed at encode time.

Also note that our SRT demuxer still outputs two \n because it's just
splitting the events and removing the timing (but not trimming the \n);
that can be changed.

Anyway, there are various ways leading to \0, \r\n, \n, \r\n\r\n, \n\n.

I think we should fix the double line breaks in our demuxer and encoder
because it's our SRT mess. Though, about trying to strip only one \n, I
think it has its place in the SRT muxer since it's likely we will find
muxed subtitles with a trailing linebreak. The patch for the muxer would
just become something like:

    if (pkt->size > 0 && pkt->data[pkt->size - 1] == '\n')
        avio_write(avf->pb, "\n", 1);       // add event separator
    else if (pkt->size > 1 && !memcmp(pkt->data+pkt->size-2, "\r\n"))
        avio_write(avf->pb, "\r\n", 1);     // add dos-like event separator
    else
        avio_write(avf->pb, "\n\n", 1);     // ends event and add separator

-- 
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/20121025/eceafe66/attachment.asc>