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

[Nicolas George]

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].

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.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121025/5a4c68d6/attachment.asc>