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

[Clément Bœsch]

On Sun, Nov 18, 2012 at 03:26:49PM +0100, Nicolas George wrote:
> Le primidi 21 brumaire, an CCXXI, Clément Bœsch a écrit :
> > Some SubRip packets might have a trailing \n, this is now taken into
> > account while adding a separator between events to avoid too much line
> > breaks.
> > 
> > Note that events in the FATE tests have more than one line breaks at the
> > end.
> 
> It makes the code significantly more complex, while assuming things about
> the packet format that are still undecided. Would you agree to leave it out
> for now, and come back to it when the other points are unresolved? The extra
> newlines are not really harmful.
> 

OK, dropped for now.

> > +    if (write_ts) {
> > +        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", 2))
> > +            avio_write(avf->pb, "\r\n", 2);     // add dos-like event separator
> > +        else
> > +            avio_write(avf->pb, "\n\n", 2);     // ends event and add separator
> > +    }
> 
> If I read things correctly, the second branch is unreachable. And testing
> the final newline to determine whether we need CRLF or LF is not enough, if
> there is no final newline: "- Hi.[\r]\n- Hello."
> 

Right, forget this for now, thanks for the review.

-- 
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/20121120/31d63f35/attachment.asc>