Re: [FFmpeg-devel] [PATCH 2/2] Move AVSubtitle from lavc to lavu

2014-12-15 Thread wm4
On Sun, 14 Dec 2014 16:47:44 +0100
Clément Bœsch u...@pkh.me wrote:

 ---
  doc/APIchanges   |   5 +++
  libavcodec/avcodec.h |  63 +++---
  libavcodec/utils.c   |  29 +++---
  libavcodec/version.h |   3 ++
  libavutil/Makefile   |   2 +
  libavutil/subtitle.c |  62 +
  libavutil/subtitle.h | 107 
 +++
  7 files changed, 191 insertions(+), 80 deletions(-)
  create mode 100644 libavutil/subtitle.c
  create mode 100644 libavutil/subtitle.h
 
 diff --git a/doc/APIchanges b/doc/APIchanges
 index ba7eae1..989794a 100644
 --- a/doc/APIchanges
 +++ b/doc/APIchanges
 @@ -15,6 +15,11 @@ libavutil: 2014-08-09
  
  API changes, most recent first:
  
 +2014-12-xx - xxx - lavc 56.16.100 / lavu 54.17.100 - lavc/avcodec.h 
 lavu/picture.h
 +  Move AVSubtitle definition from libavcodec to libavutil and add
 +  av_subtitle_*() functions which should be used instead of stack allocation
 +  and the now deprecated avsubtitle_free().
 +

should?

Can this requirement be lifted/delayed until there's actually a reason
to do so? At that point, the struct should probably be renamed, to
avoid turning valid code into subtly broken code merely by adding API
compatible implementation details.

  2014-12-xx - xxx - lavc 56.15.100 / lavu 54.16.100 - lavc/avcodec.h 
 lavu/picture.h
Move AVPicture definition from libavcodec to libavutil. This should be
transparent API and ABI wise.
 diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
 index 2c1918d..6ded06b 100644
 --- a/libavcodec/avcodec.h
 +++ b/libavcodec/avcodec.h
 @@ -40,6 +40,7 @@
  #include libavutil/picture.h
  #include libavutil/pixfmt.h
  #include libavutil/rational.h
 +#include libavutil/subtitle.h
  
  #include version.h
  
 @@ -3149,8 +3150,6 @@ typedef struct AVProfile {
  
  typedef struct AVCodecDefault AVCodecDefault;
  
 -struct AVSubtitle;
 -
  /**
   * AVCodec.
   */
 @@ -3401,61 +3400,6 @@ typedef struct AVHWAccel {
   * @}
   */
  
 -enum AVSubtitleType {
 -SUBTITLE_NONE,
 -
 -SUBTITLE_BITMAP,/// A bitmap, pict will be set
 -
 -/**
 - * Plain text, the text field must be set by the decoder and is
 - * authoritative. ass and pict fields may contain approximations.
 - */
 -SUBTITLE_TEXT,
 -
 -/**
 - * Formatted text, the ass field must be set by the decoder and is
 - * authoritative. pict and text fields may contain approximations.
 - */
 -SUBTITLE_ASS,
 -};
 -
 -#define AV_SUBTITLE_FLAG_FORCED 0x0001
 -
 -typedef struct AVSubtitleRect {
 -int x; /// top left corner  of pict, undefined when pict is not 
 set
 -int y; /// top left corner  of pict, undefined when pict is not 
 set
 -int w; /// widthof pict, undefined when pict is not 
 set
 -int h; /// height   of pict, undefined when pict is not 
 set
 -int nb_colors; /// number of colors in pict, undefined when pict is not 
 set
 -
 -/**
 - * data+linesize for the bitmap of this subtitle.
 - * can be set for text/ass as well once they are rendered
 - */
 -AVPicture pict;
 -enum AVSubtitleType type;
 -
 -char *text; /// 0 terminated plain UTF-8 text
 -
 -/**
 - * 0 terminated ASS/SSA compatible event line.
 - * The presentation of this is unaffected by the other values in this
 - * struct.
 - */
 -char *ass;
 -
 -int flags;
 -} AVSubtitleRect;
 -
 -typedef struct AVSubtitle {
 -uint16_t format; /* 0 = graphics */
 -uint32_t start_display_time; /* relative to packet pts, in ms */
 -uint32_t end_display_time; /* relative to packet pts, in ms */
 -unsigned num_rects;
 -AVSubtitleRect **rects;
 -int64_t pts;/// Same as packet pts, in AV_TIME_BASE
 -} AVSubtitle;
 -
  /**
   * If c is NULL, returns the first registered codec,
   * if c is non-NULL, returns the next registered codec after c,
 @@ -3652,12 +3596,17 @@ int avcodec_open2(AVCodecContext *avctx, const 
 AVCodec *codec, AVDictionary **op
   */
  int avcodec_close(AVCodecContext *avctx);
  
 +#if FF_API_AVSUBTITLE
  /**
   * Free all allocated data in the given subtitle struct.
 + * @deprecated use av_subtitle_*() functions (the immediate equivalent of
 + * avsubtitle_free() is av_subtitle_clear())
   *
   * @param sub AVSubtitle to free.
   */
 +attribute_deprecated
  void avsubtitle_free(AVSubtitle *sub);
 +#endif
  
  /**
   * @}
 diff --git a/libavcodec/utils.c b/libavcodec/utils.c
 index db79b67..dc0c1ec 100644
 --- a/libavcodec/utils.c
 +++ b/libavcodec/utils.c
 @@ -39,6 +39,7 @@
  #include libavutil/pixdesc.h
  #include libavutil/imgutils.h
  #include libavutil/samplefmt.h
 +#include libavutil/subtitle.h
  #include libavutil/dict.h
  #include avcodec.h
  #include libavutil/opt.h
 @@ -1270,12 +1271,6 @@ int av_codec_get_max_lowres(const AVCodec *codec)
  return 

Re: [FFmpeg-devel] [PATCH 2/2] Move AVSubtitle from lavc to lavu

2014-12-15 Thread wm4
On Mon, 15 Dec 2014 10:38:01 +0100
Clément Bœsch u...@pkh.me wrote:

 On Mon, Dec 15, 2014 at 10:23:33AM +0100, wm4 wrote:
  On Sun, 14 Dec 2014 16:47:44 +0100
  Clément Bœsch u...@pkh.me wrote:
  
   ---
doc/APIchanges   |   5 +++
libavcodec/avcodec.h |  63 +++---
libavcodec/utils.c   |  29 +++---
libavcodec/version.h |   3 ++
libavutil/Makefile   |   2 +
libavutil/subtitle.c |  62 +
libavutil/subtitle.h | 107 
   +++
7 files changed, 191 insertions(+), 80 deletions(-)
create mode 100644 libavutil/subtitle.c
create mode 100644 libavutil/subtitle.h
   
   diff --git a/doc/APIchanges b/doc/APIchanges
   index ba7eae1..989794a 100644
   --- a/doc/APIchanges
   +++ b/doc/APIchanges
   @@ -15,6 +15,11 @@ libavutil: 2014-08-09

API changes, most recent first:

   +2014-12-xx - xxx - lavc 56.16.100 / lavu 54.17.100 - lavc/avcodec.h 
   lavu/picture.h
   +  Move AVSubtitle definition from libavcodec to libavutil and add
   +  av_subtitle_*() functions which should be used instead of stack 
   allocation
   +  and the now deprecated avsubtitle_free().
   +
  
  should?
  
  Can this requirement be lifted/delayed until there's actually a reason
  to do so?
 
 The structure is unchanged for now, so yeah it's should (as a prevention
 for the future). I don't plan to add a field that soon, but it's highly
 recommended. Maybe I can change the should into must and saying that
 for now it has no impact, but will in the future.
 
  At that point, the struct should probably be renamed, to
  avoid turning valid code into subtly broken code merely by adding API
  compatible implementation details.
 
 There are actually 2 changes in this code, and maybe I should split. But
 basically the addition of the functions is kind of unrelated to the move.
 It's just a security for us for later, when we will be willing to add a
 field (if we ever do).
 
 We should have created these functions a long time ago, but the fact that
 AVSubtitle actually belong into libavutil prevented us from doing it:
 basically, if we had added these helpers, the move to libavutil would have
 been way more complicated (moving a structure definition is way easier
 than moving a structure definition AND its functions).
 
 [...]
 

My point is: barely anyone (applications) will notice this API change.
It will go unnoticed, or there will be no perceived need to update the
code. And then when you actually extend this stuff, you will start
breaking application code that worked before.

This is how it always goes. Even ffmpeg.org still links tutorials based
on ancient practices that over time became invalid, but everyone keeps
doing it because it still appears to work (but crashes in slightly
different situations).

So I think such implicit changes should be avoided. If the API
breaks, make sure applications using the old assumptions won't compile.
(There are various ways to achieve this goal; I'm just saying that it
should be a goal.)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


Re: [FFmpeg-devel] [PATCH 2/2] Move AVSubtitle from lavc to lavu

2014-12-15 Thread Clément Bœsch
On Mon, Dec 15, 2014 at 10:49:47AM +0100, wm4 wrote:
 On Mon, 15 Dec 2014 10:38:01 +0100
 Clément Bœsch u...@pkh.me wrote:
 
  On Mon, Dec 15, 2014 at 10:23:33AM +0100, wm4 wrote:
   On Sun, 14 Dec 2014 16:47:44 +0100
   Clément Bœsch u...@pkh.me wrote:
   
---
 doc/APIchanges   |   5 +++
 libavcodec/avcodec.h |  63 +++---
 libavcodec/utils.c   |  29 +++---
 libavcodec/version.h |   3 ++
 libavutil/Makefile   |   2 +
 libavutil/subtitle.c |  62 +
 libavutil/subtitle.h | 107 
+++
 7 files changed, 191 insertions(+), 80 deletions(-)
 create mode 100644 libavutil/subtitle.c
 create mode 100644 libavutil/subtitle.h

diff --git a/doc/APIchanges b/doc/APIchanges
index ba7eae1..989794a 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,11 @@ libavutil: 2014-08-09
 
 API changes, most recent first:
 
+2014-12-xx - xxx - lavc 56.16.100 / lavu 54.17.100 - 
lavc/avcodec.h lavu/picture.h
+  Move AVSubtitle definition from libavcodec to libavutil and add
+  av_subtitle_*() functions which should be used instead of stack 
allocation
+  and the now deprecated avsubtitle_free().
+
   
   should?
   
   Can this requirement be lifted/delayed until there's actually a reason
   to do so?
  
  The structure is unchanged for now, so yeah it's should (as a prevention
  for the future). I don't plan to add a field that soon, but it's highly
  recommended. Maybe I can change the should into must and saying that
  for now it has no impact, but will in the future.
  
   At that point, the struct should probably be renamed, to
   avoid turning valid code into subtly broken code merely by adding API
   compatible implementation details.
  
  There are actually 2 changes in this code, and maybe I should split. But
  basically the addition of the functions is kind of unrelated to the move.
  It's just a security for us for later, when we will be willing to add a
  field (if we ever do).
  
  We should have created these functions a long time ago, but the fact that
  AVSubtitle actually belong into libavutil prevented us from doing it:
  basically, if we had added these helpers, the move to libavutil would have
  been way more complicated (moving a structure definition is way easier
  than moving a structure definition AND its functions).
  
  [...]
  
 
 My point is: barely anyone (applications) will notice this API change.
 It will go unnoticed, or there will be no perceived need to update the
 code. And then when you actually extend this stuff, you will start
 breaking application code that worked before.

What do you suggest to make people notice the API change? Currently
AVSubtitle has one single function, which I deprecated.

So you think we should introduce a AVSubtitle2 into libavutil instead? I
don't like that that much TBH, especially since it will require me to name
the new functions av_subtitle2_* to make it consistent.

[...]

-- 
Clément B.


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


Re: [FFmpeg-devel] [PATCH 2/2] Move AVSubtitle from lavc to lavu

2014-12-15 Thread wm4
On Mon, 15 Dec 2014 11:38:58 +0100
Clément Bœsch u...@pkh.me wrote:

 On Mon, Dec 15, 2014 at 10:49:47AM +0100, wm4 wrote:
  On Mon, 15 Dec 2014 10:38:01 +0100
  Clément Bœsch u...@pkh.me wrote:
  
   On Mon, Dec 15, 2014 at 10:23:33AM +0100, wm4 wrote:
On Sun, 14 Dec 2014 16:47:44 +0100
Clément Bœsch u...@pkh.me wrote:

 ---
  doc/APIchanges   |   5 +++
  libavcodec/avcodec.h |  63 +++---
  libavcodec/utils.c   |  29 +++---
  libavcodec/version.h |   3 ++
  libavutil/Makefile   |   2 +
  libavutil/subtitle.c |  62 +
  libavutil/subtitle.h | 107 
 +++
  7 files changed, 191 insertions(+), 80 deletions(-)
  create mode 100644 libavutil/subtitle.c
  create mode 100644 libavutil/subtitle.h
 
 diff --git a/doc/APIchanges b/doc/APIchanges
 index ba7eae1..989794a 100644
 --- a/doc/APIchanges
 +++ b/doc/APIchanges
 @@ -15,6 +15,11 @@ libavutil: 2014-08-09
  
  API changes, most recent first:
  
 +2014-12-xx - xxx - lavc 56.16.100 / lavu 54.17.100 - 
 lavc/avcodec.h lavu/picture.h
 +  Move AVSubtitle definition from libavcodec to libavutil and add
 +  av_subtitle_*() functions which should be used instead of stack 
 allocation
 +  and the now deprecated avsubtitle_free().
 +

should?

Can this requirement be lifted/delayed until there's actually a reason
to do so?
   
   The structure is unchanged for now, so yeah it's should (as a prevention
   for the future). I don't plan to add a field that soon, but it's highly
   recommended. Maybe I can change the should into must and saying that
   for now it has no impact, but will in the future.
   
At that point, the struct should probably be renamed, to
avoid turning valid code into subtly broken code merely by adding API
compatible implementation details.
   
   There are actually 2 changes in this code, and maybe I should split. But
   basically the addition of the functions is kind of unrelated to the move.
   It's just a security for us for later, when we will be willing to add a
   field (if we ever do).
   
   We should have created these functions a long time ago, but the fact that
   AVSubtitle actually belong into libavutil prevented us from doing it:
   basically, if we had added these helpers, the move to libavutil would have
   been way more complicated (moving a structure definition is way easier
   than moving a structure definition AND its functions).
   
   [...]
   
  
  My point is: barely anyone (applications) will notice this API change.
  It will go unnoticed, or there will be no perceived need to update the
  code. And then when you actually extend this stuff, you will start
  breaking application code that worked before.
 
 What do you suggest to make people notice the API change? Currently
 AVSubtitle has one single function, which I deprecated.

That could be ok. You can't use the current API without using
avsubtitle_free(). (Although I'd still prefer something more explicit.)
The deprecated function must be completely removed as soon as using the
new functions is mandatory.

 So you think we should introduce a AVSubtitle2 into libavutil instead? I
 don't like that that much TBH, especially since it will require me to name
 the new functions av_subtitle2_* to make it consistent.
 

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


[FFmpeg-devel] [PATCH 2/2] Move AVSubtitle from lavc to lavu

2014-12-14 Thread Clément Bœsch
---
 doc/APIchanges   |   5 +++
 libavcodec/avcodec.h |  63 +++---
 libavcodec/utils.c   |  29 +++---
 libavcodec/version.h |   3 ++
 libavutil/Makefile   |   2 +
 libavutil/subtitle.c |  62 +
 libavutil/subtitle.h | 107 +++
 7 files changed, 191 insertions(+), 80 deletions(-)
 create mode 100644 libavutil/subtitle.c
 create mode 100644 libavutil/subtitle.h

diff --git a/doc/APIchanges b/doc/APIchanges
index ba7eae1..989794a 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,11 @@ libavutil: 2014-08-09
 
 API changes, most recent first:
 
+2014-12-xx - xxx - lavc 56.16.100 / lavu 54.17.100 - lavc/avcodec.h 
lavu/picture.h
+  Move AVSubtitle definition from libavcodec to libavutil and add
+  av_subtitle_*() functions which should be used instead of stack allocation
+  and the now deprecated avsubtitle_free().
+
 2014-12-xx - xxx - lavc 56.15.100 / lavu 54.16.100 - lavc/avcodec.h 
lavu/picture.h
   Move AVPicture definition from libavcodec to libavutil. This should be
   transparent API and ABI wise.
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 2c1918d..6ded06b 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -40,6 +40,7 @@
 #include libavutil/picture.h
 #include libavutil/pixfmt.h
 #include libavutil/rational.h
+#include libavutil/subtitle.h
 
 #include version.h
 
@@ -3149,8 +3150,6 @@ typedef struct AVProfile {
 
 typedef struct AVCodecDefault AVCodecDefault;
 
-struct AVSubtitle;
-
 /**
  * AVCodec.
  */
@@ -3401,61 +3400,6 @@ typedef struct AVHWAccel {
  * @}
  */
 
-enum AVSubtitleType {
-SUBTITLE_NONE,
-
-SUBTITLE_BITMAP,/// A bitmap, pict will be set
-
-/**
- * Plain text, the text field must be set by the decoder and is
- * authoritative. ass and pict fields may contain approximations.
- */
-SUBTITLE_TEXT,
-
-/**
- * Formatted text, the ass field must be set by the decoder and is
- * authoritative. pict and text fields may contain approximations.
- */
-SUBTITLE_ASS,
-};
-
-#define AV_SUBTITLE_FLAG_FORCED 0x0001
-
-typedef struct AVSubtitleRect {
-int x; /// top left corner  of pict, undefined when pict is not 
set
-int y; /// top left corner  of pict, undefined when pict is not 
set
-int w; /// widthof pict, undefined when pict is not 
set
-int h; /// height   of pict, undefined when pict is not 
set
-int nb_colors; /// number of colors in pict, undefined when pict is not 
set
-
-/**
- * data+linesize for the bitmap of this subtitle.
- * can be set for text/ass as well once they are rendered
- */
-AVPicture pict;
-enum AVSubtitleType type;
-
-char *text; /// 0 terminated plain UTF-8 text
-
-/**
- * 0 terminated ASS/SSA compatible event line.
- * The presentation of this is unaffected by the other values in this
- * struct.
- */
-char *ass;
-
-int flags;
-} AVSubtitleRect;
-
-typedef struct AVSubtitle {
-uint16_t format; /* 0 = graphics */
-uint32_t start_display_time; /* relative to packet pts, in ms */
-uint32_t end_display_time; /* relative to packet pts, in ms */
-unsigned num_rects;
-AVSubtitleRect **rects;
-int64_t pts;/// Same as packet pts, in AV_TIME_BASE
-} AVSubtitle;
-
 /**
  * If c is NULL, returns the first registered codec,
  * if c is non-NULL, returns the next registered codec after c,
@@ -3652,12 +3596,17 @@ int avcodec_open2(AVCodecContext *avctx, const AVCodec 
*codec, AVDictionary **op
  */
 int avcodec_close(AVCodecContext *avctx);
 
+#if FF_API_AVSUBTITLE
 /**
  * Free all allocated data in the given subtitle struct.
+ * @deprecated use av_subtitle_*() functions (the immediate equivalent of
+ * avsubtitle_free() is av_subtitle_clear())
  *
  * @param sub AVSubtitle to free.
  */
+attribute_deprecated
 void avsubtitle_free(AVSubtitle *sub);
+#endif
 
 /**
  * @}
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index db79b67..dc0c1ec 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -39,6 +39,7 @@
 #include libavutil/pixdesc.h
 #include libavutil/imgutils.h
 #include libavutil/samplefmt.h
+#include libavutil/subtitle.h
 #include libavutil/dict.h
 #include avcodec.h
 #include libavutil/opt.h
@@ -1270,12 +1271,6 @@ int av_codec_get_max_lowres(const AVCodec *codec)
 return codec-max_lowres;
 }
 
-static void get_subtitle_defaults(AVSubtitle *sub)
-{
-memset(sub, 0, sizeof(*sub));
-sub-pts = AV_NOPTS_VALUE;
-}
-
 static int get_bit_rate(AVCodecContext *ctx)
 {
 int bit_rate;
@@ -2706,7 +2701,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, 
AVSubtitle *sub,
 }
 
 *got_sub_ptr = 0;
-get_subtitle_defaults(sub);
+av_subtitle_get_defaults(sub);
 
 if ((avctx-codec-capabilities  CODEC_CAP_DELAY) || avpkt-size) {