diff mbox series

[FFmpeg-devel,v9,3/5] avformat/mov: Refactor mov_read_dvcc_dvvc to use ff_isom_parse_dvcc_dvvc

Message ID 20211205173535.2306292-4-tcChlisop0@gmail.com
State New
Headers show
Series Add support for Matroska BlockAdditionMapping elements | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

quietvoid Dec. 5, 2021, 5:35 p.m. UTC 
To avoid duplicating code. The implementation in dovi_isom is identical.

Signed-off-by: quietvoid <tcChlisop0@gmail.com>
---
 libavformat/mov.c | 50 +++++++++--------------------------------------
 1 file changed, 9 insertions(+), 41 deletions(-)

Comments

Lance Wang Dec. 7, 2021, 2:37 p.m. UTC | #1 
On Sun, Dec 05, 2021 at 06:35:33PM +0100, quietvoid wrote:
> To avoid duplicating code. The implementation in dovi_isom is identical.
> 
> Signed-off-by: quietvoid <tcChlisop0@gmail.com>
> ---
>  libavformat/mov.c | 50 +++++++++--------------------------------------
>  1 file changed, 9 insertions(+), 41 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 5c74d099da..b5f974fe05 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -55,6 +55,7 @@
>  #include "avformat.h"
>  #include "internal.h"
>  #include "avio_internal.h"
> +#include "dovi_isom.h"
>  #include "riff.h"
>  #include "isom.h"
>  #include "libavcodec/get_bits.h"
> @@ -7051,58 +7052,25 @@ static int mov_read_dmlp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  static int mov_read_dvcc_dvvc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
>      AVStream *st;
> -    uint32_t buf;
> -    AVDOVIDecoderConfigurationRecord *dovi;
> -    size_t dovi_size;
> +    uint8_t buf[ISOM_DVCC_DVVC_SIZE + AV_INPUT_BUFFER_PADDING_SIZE];
>      int ret;
> +    int64_t read_size = atom.size;
>  
>      if (c->fc->nb_streams < 1)
>          return 0;
>      st = c->fc->streams[c->fc->nb_streams-1];
>  
> -    if ((uint64_t)atom.size > (1<<30) || atom.size < 4)
> -        return AVERROR_INVALIDDATA;
> -
> -    dovi = av_dovi_alloc(&dovi_size);
> -    if (!dovi)
> -        return AVERROR(ENOMEM);
> -
> -    dovi->dv_version_major = avio_r8(pb);
> -    dovi->dv_version_minor = avio_r8(pb);
> -
> -    buf = avio_rb16(pb);
> -    dovi->dv_profile        = (buf >> 9) & 0x7f;    // 7 bits
> -    dovi->dv_level          = (buf >> 3) & 0x3f;    // 6 bits
> -    dovi->rpu_present_flag  = (buf >> 2) & 0x01;    // 1 bit
> -    dovi->el_present_flag   = (buf >> 1) & 0x01;    // 1 bit
> -    dovi->bl_present_flag   =  buf       & 0x01;    // 1 bit
> -    if (atom.size >= 24) {  // 4 + 4 + 4 * 4
> -        buf = avio_r8(pb);
> -        dovi->dv_bl_signal_compatibility_id = (buf >> 4) & 0x0f; // 4 bits
> -    } else {
> -        // 0 stands for None
> -        // Dolby Vision V1.2.93 profiles and levels
> -        dovi->dv_bl_signal_compatibility_id = 0;
> +    // At most 24 bytes
> +    if (read_size > ISOM_DVCC_DVVC_SIZE) {
> +        read_size = ISOM_DVCC_DVVC_SIZE;
>      }
>  
> -    ret = av_stream_add_side_data(st, AV_PKT_DATA_DOVI_CONF,
> -                                  (uint8_t *)dovi, dovi_size);
> -    if (ret < 0) {
> -        av_free(dovi);
> +    if ((ret = avio_read(pb, buf, read_size)) < 0)
>          return ret;
> -    }
>  
> -    av_log(c, AV_LOG_TRACE, "DOVI in dvcC/dvvC/dvwC box, version: %d.%d, profile: %d, level: %d, "
> -           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility id: %d\n",
> -           dovi->dv_version_major, dovi->dv_version_minor,
> -           dovi->dv_profile, dovi->dv_level,
> -           dovi->rpu_present_flag,
> -           dovi->el_present_flag,
> -           dovi->bl_present_flag,
> -           dovi->dv_bl_signal_compatibility_id
> -        );
> +    read_size = ret;

I prefer to check ret ! = read_size before like below:
if ((ret = avio_read(pb, buf, read_size)) != read_size)
    return ret < 0 ? ret : AVERROR(EIO);

>  
> -    return 0;
> +    return ff_isom_parse_dvcc_dvvc(c->fc, st, buf, read_size);
>  }
>  
>  static int mov_read_kind(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> -- 
> 2.34.1
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
quietvoid Dec. 7, 2021, 8:08 p.m. UTC | #2 
On Tue, Dec 7, 2021 at 9:37 AM <lance.lmwang@gmail.com> wrote:
>
> On Sun, Dec 05, 2021 at 06:35:33PM +0100, quietvoid wrote:
> > To avoid duplicating code. The implementation in dovi_isom is identical.
> >
> > Signed-off-by: quietvoid <tcChlisop0@gmail.com>
> > ---
> >  libavformat/mov.c | 50 +++++++++--------------------------------------
> >  1 file changed, 9 insertions(+), 41 deletions(-)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 5c74d099da..b5f974fe05 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -55,6 +55,7 @@
> >  #include "avformat.h"
> >  #include "internal.h"
> >  #include "avio_internal.h"
> > +#include "dovi_isom.h"
> >  #include "riff.h"
> >  #include "isom.h"
> >  #include "libavcodec/get_bits.h"
> > @@ -7051,58 +7052,25 @@ static int mov_read_dmlp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >  static int mov_read_dvcc_dvvc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >  {
> >      AVStream *st;
> > -    uint32_t buf;
> > -    AVDOVIDecoderConfigurationRecord *dovi;
> > -    size_t dovi_size;
> > +    uint8_t buf[ISOM_DVCC_DVVC_SIZE + AV_INPUT_BUFFER_PADDING_SIZE];
> >      int ret;
> > +    int64_t read_size = atom.size;
> >
> >      if (c->fc->nb_streams < 1)
> >          return 0;
> >      st = c->fc->streams[c->fc->nb_streams-1];
> >
> > -    if ((uint64_t)atom.size > (1<<30) || atom.size < 4)
> > -        return AVERROR_INVALIDDATA;
> > -
> > -    dovi = av_dovi_alloc(&dovi_size);
> > -    if (!dovi)
> > -        return AVERROR(ENOMEM);
> > -
> > -    dovi->dv_version_major = avio_r8(pb);
> > -    dovi->dv_version_minor = avio_r8(pb);
> > -
> > -    buf = avio_rb16(pb);
> > -    dovi->dv_profile        = (buf >> 9) & 0x7f;    // 7 bits
> > -    dovi->dv_level          = (buf >> 3) & 0x3f;    // 6 bits
> > -    dovi->rpu_present_flag  = (buf >> 2) & 0x01;    // 1 bit
> > -    dovi->el_present_flag   = (buf >> 1) & 0x01;    // 1 bit
> > -    dovi->bl_present_flag   =  buf       & 0x01;    // 1 bit
> > -    if (atom.size >= 24) {  // 4 + 4 + 4 * 4
> > -        buf = avio_r8(pb);
> > -        dovi->dv_bl_signal_compatibility_id = (buf >> 4) & 0x0f; // 4 bits
> > -    } else {
> > -        // 0 stands for None
> > -        // Dolby Vision V1.2.93 profiles and levels
> > -        dovi->dv_bl_signal_compatibility_id = 0;
> > +    // At most 24 bytes
> > +    if (read_size > ISOM_DVCC_DVVC_SIZE) {
> > +        read_size = ISOM_DVCC_DVVC_SIZE;
> >      }
> >
> > -    ret = av_stream_add_side_data(st, AV_PKT_DATA_DOVI_CONF,
> > -                                  (uint8_t *)dovi, dovi_size);
> > -    if (ret < 0) {
> > -        av_free(dovi);
> > +    if ((ret = avio_read(pb, buf, read_size)) < 0)
> >          return ret;
> > -    }
> >
> > -    av_log(c, AV_LOG_TRACE, "DOVI in dvcC/dvvC/dvwC box, version: %d.%d, profile: %d, level: %d, "
> > -           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility id: %d\n",
> > -           dovi->dv_version_major, dovi->dv_version_minor,
> > -           dovi->dv_profile, dovi->dv_level,
> > -           dovi->rpu_present_flag,
> > -           dovi->el_present_flag,
> > -           dovi->bl_present_flag,
> > -           dovi->dv_bl_signal_compatibility_id
> > -        );
> > +    read_size = ret;
>
> I prefer to check ret ! = read_size before like below:
> if ((ret = avio_read(pb, buf, read_size)) != read_size)
>     return ret < 0 ? ret : AVERROR(EIO);

Agreed. Will fix it.

> >
> > -    return 0;
> > +    return ff_isom_parse_dvcc_dvvc(c->fc, st, buf, read_size);
> >  }
> >
> >  static int mov_read_kind(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> > --
> > 2.34.1
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
> --
> Thanks,
> Limin Wang
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 5c74d099da..b5f974fe05 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -55,6 +55,7 @@ 
 #include "avformat.h"
 #include "internal.h"
 #include "avio_internal.h"
+#include "dovi_isom.h"
 #include "riff.h"
 #include "isom.h"
 #include "libavcodec/get_bits.h"
@@ -7051,58 +7052,25 @@  static int mov_read_dmlp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 static int mov_read_dvcc_dvvc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
     AVStream *st;
-    uint32_t buf;
-    AVDOVIDecoderConfigurationRecord *dovi;
-    size_t dovi_size;
+    uint8_t buf[ISOM_DVCC_DVVC_SIZE + AV_INPUT_BUFFER_PADDING_SIZE];
     int ret;
+    int64_t read_size = atom.size;
 
     if (c->fc->nb_streams < 1)
         return 0;
     st = c->fc->streams[c->fc->nb_streams-1];
 
-    if ((uint64_t)atom.size > (1<<30) || atom.size < 4)
-        return AVERROR_INVALIDDATA;
-
-    dovi = av_dovi_alloc(&dovi_size);
-    if (!dovi)
-        return AVERROR(ENOMEM);
-
-    dovi->dv_version_major = avio_r8(pb);
-    dovi->dv_version_minor = avio_r8(pb);
-
-    buf = avio_rb16(pb);
-    dovi->dv_profile        = (buf >> 9) & 0x7f;    // 7 bits
-    dovi->dv_level          = (buf >> 3) & 0x3f;    // 6 bits
-    dovi->rpu_present_flag  = (buf >> 2) & 0x01;    // 1 bit
-    dovi->el_present_flag   = (buf >> 1) & 0x01;    // 1 bit
-    dovi->bl_present_flag   =  buf       & 0x01;    // 1 bit
-    if (atom.size >= 24) {  // 4 + 4 + 4 * 4
-        buf = avio_r8(pb);
-        dovi->dv_bl_signal_compatibility_id = (buf >> 4) & 0x0f; // 4 bits
-    } else {
-        // 0 stands for None
-        // Dolby Vision V1.2.93 profiles and levels
-        dovi->dv_bl_signal_compatibility_id = 0;
+    // At most 24 bytes
+    if (read_size > ISOM_DVCC_DVVC_SIZE) {
+        read_size = ISOM_DVCC_DVVC_SIZE;
     }
 
-    ret = av_stream_add_side_data(st, AV_PKT_DATA_DOVI_CONF,
-                                  (uint8_t *)dovi, dovi_size);
-    if (ret < 0) {
-        av_free(dovi);
+    if ((ret = avio_read(pb, buf, read_size)) < 0)
         return ret;
-    }
 
-    av_log(c, AV_LOG_TRACE, "DOVI in dvcC/dvvC/dvwC box, version: %d.%d, profile: %d, level: %d, "
-           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility id: %d\n",
-           dovi->dv_version_major, dovi->dv_version_minor,
-           dovi->dv_profile, dovi->dv_level,
-           dovi->rpu_present_flag,
-           dovi->el_present_flag,
-           dovi->bl_present_flag,
-           dovi->dv_bl_signal_compatibility_id
-        );
+    read_size = ret;
 
-    return 0;
+    return ff_isom_parse_dvcc_dvvc(c->fc, st, buf, read_size);
 }
 
 static int mov_read_kind(MOVContext *c, AVIOContext *pb, MOVAtom atom)