Re: [FFmpeg-devel] [PATCH 10/17] avcodec/webvttdec: add some memory checks

2014-09-21 Thread Nicolas George 
Le jour de la Raison, an CCXXII, Clément Bœsch a écrit :
 ---
  libavcodec/webvttdec.c | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

Patches 5-10 look right, except most of them could benefit from the
factorization.

Regards,

-- 
  Nicolas George
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 10/17] avcodec/webvttdec: add some memory checks

2014-09-21 Thread Clément Bœsch 
On Sun, Sep 21, 2014 at 11:20:26AM +0200, Nicolas George wrote:
 Le jour de la Raison, an CCXXII, Clément Bœsch a écrit :
  ---
   libavcodec/webvttdec.c | 10 --
   1 file changed, 8 insertions(+), 2 deletions(-)
 
 Patches 5-10 look right, except most of them could benefit from the
 factorization.
 

So I did that and pushed that part of the patchset, adjusted to use a
bprint helper.

Unfortunately, this raises a warning because av_bprint_is_complete()
doesn't take a const parameter. I was too lazy to actually fix the API,
but didn't want to cast to hide the warning. If you can send a patch to
fix the bprint API that would be really appreciated. Thanks :)

Thank you for your review,

-- 
Clément B.


pgpWLUgDZYJzx.pgp
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 10/17] avcodec/webvttdec: add some memory checks

2014-09-21 Thread Clément Bœsch 
On Sun, Sep 21, 2014 at 07:19:43PM +0200, Nicolas George wrote:
 Le jour de la Récompense, an CCXXII, Clément Bœsch a écrit :
  So I did that and pushed that part of the patchset, adjusted to use a
  bprint helper.
  
  Unfortunately, this raises a warning because av_bprint_is_complete()
  doesn't take a const parameter. I was too lazy to actually fix the API,
  but didn't want to cast to hide the warning. If you can send a patch to
  fix the bprint API that would be really appreciated. Thanks :)
 
 Pushed the attached patch to my tree: Michael can pull if no objection.
 

Doesn't this need at least a minor bump?

 But you could have not needed it and make the whole set even simpler if you
 had added av_bprint_finalize() to the helper itself.

Not really; in SAMI at least the buffer is allocated only once, and
finalized only at the end. This behaviour could be adopted in other
text subtitles demuxers.

 Even better, *got_sub = sub-num_rects  0 could have been factored
 too.

This chunk will change later

 
 Regards,
 
 -- 
   Nicolas George

 From c0394614bacb38a3a303e80b246a76f37a28b391 Mon Sep 17 00:00:00 2001
 From: Nicolas George geo...@nsup.org
 Date: Sun, 21 Sep 2014 19:03:33 +0200
 Subject: [PATCH] lavu/bprint: add const to av_bprint_is_complete() argument.
 
 Signed-off-by: Nicolas George geo...@nsup.org
 ---
  libavutil/bprint.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/libavutil/bprint.h b/libavutil/bprint.h
 index 839ec1e..d1682fc 100644
 --- a/libavutil/bprint.h
 +++ b/libavutil/bprint.h
 @@ -179,7 +179,7 @@ void av_bprint_clear(AVBPrint *buf);
   * It may have been truncated due to a memory allocation failure
   * or the size_max limit (compare size and size_max if necessary).
   */
 -static inline int av_bprint_is_complete(AVBPrint *buf)
 +static inline int av_bprint_is_complete(const AVBPrint *buf)
  {
  return buf-len  buf-size;
  }
 -- 
 2.1.0
 


-- 
Clément B.


pgp5SNLzhaqdv.pgp
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 10/17] avcodec/webvttdec: add some memory checks

2014-09-21 Thread Nicolas George 
Le jour de la Récompense, an CCXXII, Clément Bœsch a écrit :
 Doesn't this need at least a minor bump?

I consider it a bug fix, not an API change. Can you imagine a case where
people would need to check for it?

 Not really; in SAMI at least the buffer is allocated only once, and
 finalized only at the end. This behaviour could be adopted in other
 text subtitles demuxers.

That makes sense, but you still could do it with an extra argument to select
between finalize, clear and nothing.

 This chunk will change later

Ok.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 10/17] avcodec/webvttdec: add some memory checks

2014-09-21 Thread Clément Bœsch 
On Sun, Sep 21, 2014 at 07:36:19PM +0200, Nicolas George wrote:
 Le jour de la Récompense, an CCXXII, Clément Bœsch a écrit :
  Doesn't this need at least a minor bump?
 
 I consider it a bug fix, not an API change. Can you imagine a case where
 people would need to check for it?
 

One case that might break is when used as a callback. That's very unlikely
to happen, but it's still a change in the public API. You might get some
rainbow colors from http://upstream-tracker.org/ typically.

  Not really; in SAMI at least the buffer is allocated only once, and
  finalized only at the end. This behaviour could be adopted in other
  text subtitles demuxers.
 
 That makes sense, but you still could do it with an extra argument to select
 between finalize, clear and nothing.
 

Yeah maybe. That function will be dropped anyway, it's just in order to
ease the transition to the next API.

[...]

-- 
Clément B.


pgpaH5UGeQuM7.pgp
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 10/17] avcodec/webvttdec: add some memory checks

2014-09-21 Thread Nicolas George 
Le jour de la Récompense, an CCXXII, Clément Bœsch a écrit :
 One case that might break is when used as a callback. That's very unlikely
 to happen, but it's still a change in the public API. You might get some
 rainbow colors from http://upstream-tracker.org/ typically.

All right, but only micro IMHO. Pushed to my tree and attached.

 Yeah maybe. That function will be dropped anyway, it's just in order to
 ease the transition to the next API.

Since it is already pushed, there is nothing to add.

Regards,

-- 
  Nicolas George
From c0394614bacb38a3a303e80b246a76f37a28b391 Mon Sep 17 00:00:00 2001
From: Nicolas George geo...@nsup.org
Date: Sun, 21 Sep 2014 19:03:33 +0200
Subject: [PATCH] lavu/bprint: add const to av_bprint_is_complete() argument.

Signed-off-by: Nicolas George geo...@nsup.org
---
 libavutil/bprint.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/bprint.h b/libavutil/bprint.h
index 839ec1e..d1682fc 100644
--- a/libavutil/bprint.h
+++ b/libavutil/bprint.h
@@ -179,7 +179,7 @@ void av_bprint_clear(AVBPrint *buf);
  * It may have been truncated due to a memory allocation failure
  * or the size_max limit (compare size and size_max if necessary).
  */
-static inline int av_bprint_is_complete(AVBPrint *buf)
+static inline int av_bprint_is_complete(const AVBPrint *buf)
 {
 return buf-len  buf-size;
 }
-- 
2.1.0



signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 10/17] avcodec/webvttdec: add some memory checks

2014-09-21 Thread Michael Niedermayer 
On Sun, Sep 21, 2014 at 07:43:39PM +0200, Nicolas George wrote:
 Le jour de la Récompense, an CCXXII, Clément Bœsch a écrit :
  One case that might break is when used as a callback. That's very unlikely
  to happen, but it's still a change in the public API. You might get some
  rainbow colors from http://upstream-tracker.org/ typically.
 
 All right, but only micro IMHO. Pushed to my tree and attached.

merged

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 10/17] avcodec/webvttdec: add some memory checks

2014-09-20 Thread Clément Bœsch 
---
 libavcodec/webvttdec.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/libavcodec/webvttdec.c b/libavcodec/webvttdec.c
index 6b86bed..411abf0 100644
--- a/libavcodec/webvttdec.c
+++ b/libavcodec/webvttdec.c
@@ -74,6 +74,7 @@ static int webvtt_event_to_ass(AVBPrint *buf, const char *p)
 static int webvtt_decode_frame(AVCodecContext *avctx,
void *data, int *got_sub_ptr, AVPacket *avpkt)
 {
+int ret = 0;
 AVSubtitle *sub = data;
 const char *ptr = avpkt-data;
 AVBPrint buf;
@@ -83,10 +84,15 @@ static int webvtt_decode_frame(AVCodecContext *avctx,
 int ts_start = av_rescale_q(avpkt-pts, avctx-time_base, 
(AVRational){1,100});
 int ts_duration  = avpkt-duration != -1 ?
av_rescale_q(avpkt-duration, avctx-time_base, 
(AVRational){1,100}) : -1;
-ff_ass_add_rect(sub, buf.str, ts_start, ts_duration, 0);
+if (!av_bprint_is_complete(buf))
+ret = AVERROR(ENOMEM);
+else
+ret = ff_ass_add_rect(sub, buf.str, ts_start, ts_duration, 0);
 }
-*got_sub_ptr = sub-num_rects  0;
 av_bprint_finalize(buf, NULL);
+if (ret  0)
+return ret;
+*got_sub_ptr = sub-num_rects  0;
 return avpkt-size;
 }
 
-- 
2.1.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel