diff mbox series

[FFmpeg-devel,02/20] avformat/matroskaenc: Improve writing Projection

Message ID 20200101005837.11356-3-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series Matroska muxer patches
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt Jan. 1, 2020, 12:58 a.m. UTC
The Matroska Projection master element has such a small maximum length
that it can always be written with a length field of length one.
So it is unnecessary to first write the element into a dynamic buffer to
get the accurate length in order not to waste bytes on the length field.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/matroskaenc.c | 48 +++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

Comments

Carl Eugen Hoyos Jan. 1, 2020, 3:53 a.m. UTC | #1
Am Mi., 1. Jan. 2020 um 01:59 Uhr schrieb Andreas Rheinhardt
<andreas.rheinhardt@gmail.com>:
>
> The Matroska Projection master element has such a small maximum length
> that it can always be written with a length field of length one.
> So it is unnecessary to first write the element into a dynamic buffer to
> get the accurate length in order not to waste bytes on the length field.

"Improve" or "Simplify"?
Andreas Rheinhardt Jan. 1, 2020, 6:04 a.m. UTC | #2
On Wed, Jan 1, 2020 at 4:53 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Am Mi., 1. Jan. 2020 um 01:59 Uhr schrieb Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com>:
> >
> > The Matroska Projection master element has such a small maximum length
> > that it can always be written with a length field of length one.
> > So it is unnecessary to first write the element into a dynamic buffer to
> > get the accurate length in order not to waste bytes on the length field.
>
> "Improve" or "Simplify"?
>
>
Both are applicable.

- Andreas
Andreas Rheinhardt Jan. 14, 2020, 6:15 p.m. UTC | #3
On Wed, Jan 1, 2020 at 1:59 AM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> The Matroska Projection master element has such a small maximum length
> that it can always be written with a length field of length one.
> So it is unnecessary to first write the element into a dynamic buffer to
> get the accurate length in order not to waste bytes on the length field.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/matroskaenc.c | 48 +++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 27 deletions(-)
>
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 9cf840c9be..bbf9b55e78 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -947,10 +947,8 @@ static int mkv_write_video_projection(AVFormatContext
> *s, AVIOContext *pb,
>                                        AVStream *st)
>  {
>      AVIOContext b;
> -    AVIOContext *dyn_cp;
> +    ebml_master projection;
>      int side_data_size = 0;
> -    int ret, projection_size;
> -    uint8_t *projection_ptr;
>      uint8_t private[20];
>
>      const AVSphericalMapping *spherical =
> @@ -960,62 +958,58 @@ static int
> mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb,
>      if (!side_data_size)
>          return 0;
>
> -    ret = avio_open_dyn_buf(&dyn_cp);
> -    if (ret < 0)
> -        return ret;
> +    if (spherical->projection != AV_SPHERICAL_EQUIRECTANGULAR      &&
> +        spherical->projection != AV_SPHERICAL_EQUIRECTANGULAR_TILE &&
> +        spherical->projection != AV_SPHERICAL_CUBEMAP) {
> +        av_log(s, AV_LOG_WARNING, "Unknown projection type\n");
> +        return 0;
> +    }
> +
> +    // Maximally 4 8-byte elements with id-length 2 + 1 byte length field
> +    // and the private data of the AV_SPHERICAL_EQUIRECTANGULAR_TILE case
> +    projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION,
> +                                   4 * (2 + 1 + 8) + (2 + 1 + 20));
>
>      switch (spherical->projection) {
>      case AV_SPHERICAL_EQUIRECTANGULAR:
> -        put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE,
> +        put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE,
>                        MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR);
>          break;
>      case AV_SPHERICAL_EQUIRECTANGULAR_TILE:
>          ffio_init_context(&b, private, 20, 1, NULL, NULL, NULL, NULL);
> -        put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE,
> +        put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE,
>                        MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR);
>          avio_wb32(&b, 0); // version + flags
>          avio_wb32(&b, spherical->bound_top);
>          avio_wb32(&b, spherical->bound_bottom);
>          avio_wb32(&b, spherical->bound_left);
>          avio_wb32(&b, spherical->bound_right);
> -        put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
> +        put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
>                          private, avio_tell(&b));
>          break;
>      case AV_SPHERICAL_CUBEMAP:
>          ffio_init_context(&b, private, 12, 1, NULL, NULL, NULL, NULL);
> -        put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE,
> +        put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE,
>                        MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP);
>          avio_wb32(&b, 0); // version + flags
>          avio_wb32(&b, 0); // layout
>          avio_wb32(&b, spherical->padding);
> -        put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
> +        put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
>                          private, avio_tell(&b));
>          break;
> -    default:
> -        av_log(s, AV_LOG_WARNING, "Unknown projection type\n");
> -        goto end;
>      }
>
>      if (spherical->yaw)
> -        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW,
> +        put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW,
>                         (double) spherical->yaw   / (1 << 16));
>      if (spherical->pitch)
> -        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH,
> +        put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH,
>                         (double) spherical->pitch / (1 << 16));
>      if (spherical->roll)
> -        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL,
> +        put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL,
>                         (double) spherical->roll  / (1 << 16));
>
> -end:
> -    projection_size = avio_close_dyn_buf(dyn_cp, &projection_ptr);
> -    if (projection_size) {
> -        ebml_master projection = start_ebml_master(pb,
> -
>  MATROSKA_ID_VIDEOPROJECTION,
> -                                                   projection_size);
> -        avio_write(pb, projection_ptr, projection_size);
> -        end_ebml_master(pb, projection);
> -    }
> -    av_freep(&projection_ptr);
> +    end_ebml_master(pb, projection);
>
>      return 0;
>  }
> --
> 2.20.1
>
>
Ping.

- Andreas
James Almer Jan. 27, 2020, 7:42 p.m. UTC | #4
On 12/31/2019 9:58 PM, Andreas Rheinhardt wrote:
> The Matroska Projection master element has such a small maximum length
> that it can always be written with a length field of length one.
> So it is unnecessary to first write the element into a dynamic buffer to
> get the accurate length in order not to waste bytes on the length field.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/matroskaenc.c | 48 +++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 9cf840c9be..bbf9b55e78 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -947,10 +947,8 @@ static int mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb,
>                                        AVStream *st)
>  {
>      AVIOContext b;
> -    AVIOContext *dyn_cp;
> +    ebml_master projection;
>      int side_data_size = 0;
> -    int ret, projection_size;
> -    uint8_t *projection_ptr;
>      uint8_t private[20];
>  
>      const AVSphericalMapping *spherical =
> @@ -960,62 +958,58 @@ static int mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb,
>      if (!side_data_size)
>          return 0;
>  
> -    ret = avio_open_dyn_buf(&dyn_cp);
> -    if (ret < 0)
> -        return ret;
> +    if (spherical->projection != AV_SPHERICAL_EQUIRECTANGULAR      &&
> +        spherical->projection != AV_SPHERICAL_EQUIRECTANGULAR_TILE &&
> +        spherical->projection != AV_SPHERICAL_CUBEMAP) {
> +        av_log(s, AV_LOG_WARNING, "Unknown projection type\n");
> +        return 0;
> +    }
> +
> +    // Maximally 4 8-byte elements with id-length 2 + 1 byte length field
> +    // and the private data of the AV_SPHERICAL_EQUIRECTANGULAR_TILE case
> +    projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION,
> +                                   4 * (2 + 1 + 8) + (2 + 1 + 20));
>  
>      switch (spherical->projection) {
>      case AV_SPHERICAL_EQUIRECTANGULAR:
> -        put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE,
> +        put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE,
>                        MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR);
>          break;
>      case AV_SPHERICAL_EQUIRECTANGULAR_TILE:
>          ffio_init_context(&b, private, 20, 1, NULL, NULL, NULL, NULL);
> -        put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE,
> +        put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE,
>                        MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR);
>          avio_wb32(&b, 0); // version + flags
>          avio_wb32(&b, spherical->bound_top);
>          avio_wb32(&b, spherical->bound_bottom);
>          avio_wb32(&b, spherical->bound_left);
>          avio_wb32(&b, spherical->bound_right);
> -        put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
> +        put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
>                          private, avio_tell(&b));
>          break;
>      case AV_SPHERICAL_CUBEMAP:
>          ffio_init_context(&b, private, 12, 1, NULL, NULL, NULL, NULL);
> -        put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE,
> +        put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE,
>                        MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP);
>          avio_wb32(&b, 0); // version + flags
>          avio_wb32(&b, 0); // layout
>          avio_wb32(&b, spherical->padding);
> -        put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
> +        put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
>                          private, avio_tell(&b));
>          break;
> -    default:
> -        av_log(s, AV_LOG_WARNING, "Unknown projection type\n");
> -        goto end;

Changed this to av_assert0(0) since the check you added above should
ensure a "default" path is never taken here, and pushed. Thanks.

>      }
>  
>      if (spherical->yaw)
> -        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW,
> +        put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW,
>                         (double) spherical->yaw   / (1 << 16));
>      if (spherical->pitch)
> -        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH,
> +        put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH,
>                         (double) spherical->pitch / (1 << 16));
>      if (spherical->roll)
> -        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL,
> +        put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL,
>                         (double) spherical->roll  / (1 << 16));
>  
> -end:
> -    projection_size = avio_close_dyn_buf(dyn_cp, &projection_ptr);
> -    if (projection_size) {
> -        ebml_master projection = start_ebml_master(pb,
> -                                                   MATROSKA_ID_VIDEOPROJECTION,
> -                                                   projection_size);
> -        avio_write(pb, projection_ptr, projection_size);
> -        end_ebml_master(pb, projection);
> -    }
> -    av_freep(&projection_ptr);
> +    end_ebml_master(pb, projection);
>  
>      return 0;
>  }
>
diff mbox series

Patch

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 9cf840c9be..bbf9b55e78 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -947,10 +947,8 @@  static int mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb,
                                       AVStream *st)
 {
     AVIOContext b;
-    AVIOContext *dyn_cp;
+    ebml_master projection;
     int side_data_size = 0;
-    int ret, projection_size;
-    uint8_t *projection_ptr;
     uint8_t private[20];
 
     const AVSphericalMapping *spherical =
@@ -960,62 +958,58 @@  static int mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb,
     if (!side_data_size)
         return 0;
 
-    ret = avio_open_dyn_buf(&dyn_cp);
-    if (ret < 0)
-        return ret;
+    if (spherical->projection != AV_SPHERICAL_EQUIRECTANGULAR      &&
+        spherical->projection != AV_SPHERICAL_EQUIRECTANGULAR_TILE &&
+        spherical->projection != AV_SPHERICAL_CUBEMAP) {
+        av_log(s, AV_LOG_WARNING, "Unknown projection type\n");
+        return 0;
+    }
+
+    // Maximally 4 8-byte elements with id-length 2 + 1 byte length field
+    // and the private data of the AV_SPHERICAL_EQUIRECTANGULAR_TILE case
+    projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION,
+                                   4 * (2 + 1 + 8) + (2 + 1 + 20));
 
     switch (spherical->projection) {
     case AV_SPHERICAL_EQUIRECTANGULAR:
-        put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE,
+        put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE,
                       MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR);
         break;
     case AV_SPHERICAL_EQUIRECTANGULAR_TILE:
         ffio_init_context(&b, private, 20, 1, NULL, NULL, NULL, NULL);
-        put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE,
+        put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE,
                       MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR);
         avio_wb32(&b, 0); // version + flags
         avio_wb32(&b, spherical->bound_top);
         avio_wb32(&b, spherical->bound_bottom);
         avio_wb32(&b, spherical->bound_left);
         avio_wb32(&b, spherical->bound_right);
-        put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
+        put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
                         private, avio_tell(&b));
         break;
     case AV_SPHERICAL_CUBEMAP:
         ffio_init_context(&b, private, 12, 1, NULL, NULL, NULL, NULL);
-        put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE,
+        put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE,
                       MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP);
         avio_wb32(&b, 0); // version + flags
         avio_wb32(&b, 0); // layout
         avio_wb32(&b, spherical->padding);
-        put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
+        put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE,
                         private, avio_tell(&b));
         break;
-    default:
-        av_log(s, AV_LOG_WARNING, "Unknown projection type\n");
-        goto end;
     }
 
     if (spherical->yaw)
-        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW,
+        put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW,
                        (double) spherical->yaw   / (1 << 16));
     if (spherical->pitch)
-        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH,
+        put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH,
                        (double) spherical->pitch / (1 << 16));
     if (spherical->roll)
-        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL,
+        put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL,
                        (double) spherical->roll  / (1 << 16));
 
-end:
-    projection_size = avio_close_dyn_buf(dyn_cp, &projection_ptr);
-    if (projection_size) {
-        ebml_master projection = start_ebml_master(pb,
-                                                   MATROSKA_ID_VIDEOPROJECTION,
-                                                   projection_size);
-        avio_write(pb, projection_ptr, projection_size);
-        end_ebml_master(pb, projection);
-    }
-    av_freep(&projection_ptr);
+    end_ebml_master(pb, projection);
 
     return 0;
 }