diff mbox series

[FFmpeg-devel,v8,4/5] avformat/movenc: Refactor mov_write_dvcc_dvvc_tag to use ff_isom_put_dvcc_dvvc

Message ID 20211204010905.2258652-5-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. 4, 2021, 1:09 a.m. UTC 
From: quietvoid <tcchlisop0@gmail.com>

Improves code legibility by not using bit shifts.
Also avoids duplicating the dvcC/dvvC ISOM box writing code.

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

Comments

Lance Wang Dec. 4, 2021, 1:19 a.m. UTC | #1 
On Sat, Dec 04, 2021 at 02:09:04AM +0100, quietvoid wrote:
> From: quietvoid <tcchlisop0@gmail.com>
> 
> Improves code legibility by not using bit shifts.
> Also avoids duplicating the dvcC/dvvC ISOM box writing code.
> 
> Signed-off-by: quietvoid <tcChlisop0@gmail.com>
> ---
>  libavformat/movenc.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 38ff90833a..79f7d70747 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -27,6 +27,7 @@
>  #include "movenc.h"
>  #include "avformat.h"
>  #include "avio_internal.h"
> +#include "dovi_isom.h"
>  #include "riff.h"
>  #include "avio.h"
>  #include "isom.h"
> @@ -1911,6 +1912,9 @@ static int mov_write_sv3d_tag(AVFormatContext *s, AVIOContext *pb, AVSphericalMa
>  
>  static int mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb, AVDOVIDecoderConfigurationRecord *dovi)
>  {
> +    uint8_t buf[ISOM_DVCC_DVVC_SIZE];
> +    int size;
> +
>      avio_wb32(pb, 32); /* size = 8 + 24 */
>      if (dovi->dv_profile > 10)
>          ffio_wfourcc(pb, "dvwC");
> @@ -1918,23 +1922,11 @@ static int mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb, AVDOVIDe
>          ffio_wfourcc(pb, "dvvC");
>      else
>          ffio_wfourcc(pb, "dvcC");
> -    avio_w8(pb, dovi->dv_version_major);
> -    avio_w8(pb, dovi->dv_version_minor);
> -    avio_wb16(pb, (dovi->dv_profile << 9) | (dovi->dv_level << 3) |
> -              (dovi->rpu_present_flag << 2) | (dovi->el_present_flag << 1) |
> -              dovi->bl_present_flag);
> -    avio_wb32(pb, (dovi->dv_bl_signal_compatibility_id << 28) | 0);
> -
> -    ffio_fill(pb, 0, 4 * 4); /* reserved */
> -    av_log(s, AV_LOG_DEBUG, "DOVI in %s box, version: %d.%d, profile: %d, level: %d, "
> -           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility id: %d\n",
> -           dovi->dv_profile > 10 ? "dvwC" : (dovi->dv_profile > 7 ? "dvvC" : "dvcC"),
> -           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);
> +
> +    size = ff_isom_put_dvcc_dvvc(s, buf, sizeof(buf), dovi);

I think you need check the return of ff_isom_put_dvcc_dvvc(). 

> +
> +    avio_write(pb, buf, size);
> +
>      return 32; /* 8 + 24 */
>  }
>  
> -- 
> 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. 4, 2021, 1:27 a.m. UTC | #2 
On Fri, Dec 3, 2021 at 8:19 PM <lance.lmwang@gmail.com> wrote:

> On Sat, Dec 04, 2021 at 02:09:04AM +0100, quietvoid wrote:
> > From: quietvoid <tcchlisop0@gmail.com>
> >
> > Improves code legibility by not using bit shifts.
> > Also avoids duplicating the dvcC/dvvC ISOM box writing code.
> >
> > Signed-off-by: quietvoid <tcChlisop0@gmail.com>
> > ---
> >  libavformat/movenc.c | 26 +++++++++-----------------
> >  1 file changed, 9 insertions(+), 17 deletions(-)
> >
> > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > index 38ff90833a..79f7d70747 100644
> > --- a/libavformat/movenc.c
> > +++ b/libavformat/movenc.c
> > @@ -27,6 +27,7 @@
> >  #include "movenc.h"
> >  #include "avformat.h"
> >  #include "avio_internal.h"
> > +#include "dovi_isom.h"
> >  #include "riff.h"
> >  #include "avio.h"
> >  #include "isom.h"
> > @@ -1911,6 +1912,9 @@ static int mov_write_sv3d_tag(AVFormatContext *s,
> AVIOContext *pb, AVSphericalMa
> >
> >  static int mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb,
> AVDOVIDecoderConfigurationRecord *dovi)
> >  {
> > +    uint8_t buf[ISOM_DVCC_DVVC_SIZE];
> > +    int size;
> > +
> >      avio_wb32(pb, 32); /* size = 8 + 24 */
> >      if (dovi->dv_profile > 10)
> >          ffio_wfourcc(pb, "dvwC");
> > @@ -1918,23 +1922,11 @@ static int
> mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb, AVDOVIDe
> >          ffio_wfourcc(pb, "dvvC");
> >      else
> >          ffio_wfourcc(pb, "dvcC");
> > -    avio_w8(pb, dovi->dv_version_major);
> > -    avio_w8(pb, dovi->dv_version_minor);
> > -    avio_wb16(pb, (dovi->dv_profile << 9) | (dovi->dv_level << 3) |
> > -              (dovi->rpu_present_flag << 2) | (dovi->el_present_flag <<
> 1) |
> > -              dovi->bl_present_flag);
> > -    avio_wb32(pb, (dovi->dv_bl_signal_compatibility_id << 28) | 0);
> > -
> > -    ffio_fill(pb, 0, 4 * 4); /* reserved */
> > -    av_log(s, AV_LOG_DEBUG, "DOVI in %s box, version: %d.%d, profile:
> %d, level: %d, "
> > -           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility id:
> %d\n",
> > -           dovi->dv_profile > 10 ? "dvwC" : (dovi->dv_profile > 7 ?
> "dvvC" : "dvcC"),
> > -           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);
> > +
> > +    size = ff_isom_put_dvcc_dvvc(s, buf, sizeof(buf), dovi);
>
> I think you need check the return of ff_isom_put_dvcc_dvvc().
>
>
Wouldn't it necessarily return the constant ISOM_DVCC_DVVC_SIZE in this
case?
Since the only error condition is if the passed size is lower than that,
but the buffer is constant sized.

And the PutBitContext, being flushed, would return the same value.


> > +
> > +    avio_write(pb, buf, size);
> > +
> >      return 32; /* 8 + 24 */
> >  }
> >
> > --
> > 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".
>
Lance Wang Dec. 4, 2021, 1:39 a.m. UTC | #3 
On Fri, Dec 03, 2021 at 08:27:06PM -0500, quietvoid wrote:
> On Fri, Dec 3, 2021 at 8:19 PM <lance.lmwang@gmail.com> wrote:
> 
> > On Sat, Dec 04, 2021 at 02:09:04AM +0100, quietvoid wrote:
> > > From: quietvoid <tcchlisop0@gmail.com>
> > >
> > > Improves code legibility by not using bit shifts.
> > > Also avoids duplicating the dvcC/dvvC ISOM box writing code.
> > >
> > > Signed-off-by: quietvoid <tcChlisop0@gmail.com>
> > > ---
> > >  libavformat/movenc.c | 26 +++++++++-----------------
> > >  1 file changed, 9 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > > index 38ff90833a..79f7d70747 100644
> > > --- a/libavformat/movenc.c
> > > +++ b/libavformat/movenc.c
> > > @@ -27,6 +27,7 @@
> > >  #include "movenc.h"
> > >  #include "avformat.h"
> > >  #include "avio_internal.h"
> > > +#include "dovi_isom.h"
> > >  #include "riff.h"
> > >  #include "avio.h"
> > >  #include "isom.h"
> > > @@ -1911,6 +1912,9 @@ static int mov_write_sv3d_tag(AVFormatContext *s,
> > AVIOContext *pb, AVSphericalMa
> > >
> > >  static int mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb,
> > AVDOVIDecoderConfigurationRecord *dovi)
> > >  {
> > > +    uint8_t buf[ISOM_DVCC_DVVC_SIZE];
> > > +    int size;
> > > +
> > >      avio_wb32(pb, 32); /* size = 8 + 24 */
> > >      if (dovi->dv_profile > 10)
> > >          ffio_wfourcc(pb, "dvwC");
> > > @@ -1918,23 +1922,11 @@ static int
> > mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb, AVDOVIDe
> > >          ffio_wfourcc(pb, "dvvC");
> > >      else
> > >          ffio_wfourcc(pb, "dvcC");
> > > -    avio_w8(pb, dovi->dv_version_major);
> > > -    avio_w8(pb, dovi->dv_version_minor);
> > > -    avio_wb16(pb, (dovi->dv_profile << 9) | (dovi->dv_level << 3) |
> > > -              (dovi->rpu_present_flag << 2) | (dovi->el_present_flag <<
> > 1) |
> > > -              dovi->bl_present_flag);
> > > -    avio_wb32(pb, (dovi->dv_bl_signal_compatibility_id << 28) | 0);
> > > -
> > > -    ffio_fill(pb, 0, 4 * 4); /* reserved */
> > > -    av_log(s, AV_LOG_DEBUG, "DOVI in %s box, version: %d.%d, profile:
> > %d, level: %d, "
> > > -           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility id:
> > %d\n",
> > > -           dovi->dv_profile > 10 ? "dvwC" : (dovi->dv_profile > 7 ?
> > "dvvC" : "dvcC"),
> > > -           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);
> > > +
> > > +    size = ff_isom_put_dvcc_dvvc(s, buf, sizeof(buf), dovi);
> >
> > I think you need check the return of ff_isom_put_dvcc_dvvc().
> >
> >
> Wouldn't it necessarily return the constant ISOM_DVCC_DVVC_SIZE in this
> case?

yes, now it'll return negative error code in one error case. Why the out parameter is
using array instead of pointer, as the size of buffer is one of parameters also.
So if it's array, why it's necessary to check the size?

> Since the only error condition is if the passed size is lower than that,
> but the buffer is constant sized.
> 
> And the PutBitContext, being flushed, would return the same value.
> 
> 
> > > +
> > > +    avio_write(pb, buf, size);
> > > +
> > >      return 32; /* 8 + 24 */
> > >  }
> > >
> > > --
> > > 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".
> >
> _______________________________________________
> 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. 4, 2021, 1:51 a.m. UTC | #4 
On Fri, Dec 3, 2021 at 8:40 PM <lance.lmwang@gmail.com> wrote:

> On Fri, Dec 03, 2021 at 08:27:06PM -0500, quietvoid wrote:
> > On Fri, Dec 3, 2021 at 8:19 PM <lance.lmwang@gmail.com> wrote:
> >
> > > On Sat, Dec 04, 2021 at 02:09:04AM +0100, quietvoid wrote:
> > > > From: quietvoid <tcchlisop0@gmail.com>
> > > >
> > > > Improves code legibility by not using bit shifts.
> > > > Also avoids duplicating the dvcC/dvvC ISOM box writing code.
> > > >
> > > > Signed-off-by: quietvoid <tcChlisop0@gmail.com>
> > > > ---
> > > >  libavformat/movenc.c | 26 +++++++++-----------------
> > > >  1 file changed, 9 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > > > index 38ff90833a..79f7d70747 100644
> > > > --- a/libavformat/movenc.c
> > > > +++ b/libavformat/movenc.c
> > > > @@ -27,6 +27,7 @@
> > > >  #include "movenc.h"
> > > >  #include "avformat.h"
> > > >  #include "avio_internal.h"
> > > > +#include "dovi_isom.h"
> > > >  #include "riff.h"
> > > >  #include "avio.h"
> > > >  #include "isom.h"
> > > > @@ -1911,6 +1912,9 @@ static int mov_write_sv3d_tag(AVFormatContext
> *s,
> > > AVIOContext *pb, AVSphericalMa
> > > >
> > > >  static int mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext
> *pb,
> > > AVDOVIDecoderConfigurationRecord *dovi)
> > > >  {
> > > > +    uint8_t buf[ISOM_DVCC_DVVC_SIZE];
> > > > +    int size;
> > > > +
> > > >      avio_wb32(pb, 32); /* size = 8 + 24 */
> > > >      if (dovi->dv_profile > 10)
> > > >          ffio_wfourcc(pb, "dvwC");
> > > > @@ -1918,23 +1922,11 @@ static int
> > > mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb, AVDOVIDe
> > > >          ffio_wfourcc(pb, "dvvC");
> > > >      else
> > > >          ffio_wfourcc(pb, "dvcC");
> > > > -    avio_w8(pb, dovi->dv_version_major);
> > > > -    avio_w8(pb, dovi->dv_version_minor);
> > > > -    avio_wb16(pb, (dovi->dv_profile << 9) | (dovi->dv_level << 3) |
> > > > -              (dovi->rpu_present_flag << 2) |
> (dovi->el_present_flag <<
> > > 1) |
> > > > -              dovi->bl_present_flag);
> > > > -    avio_wb32(pb, (dovi->dv_bl_signal_compatibility_id << 28) | 0);
> > > > -
> > > > -    ffio_fill(pb, 0, 4 * 4); /* reserved */
> > > > -    av_log(s, AV_LOG_DEBUG, "DOVI in %s box, version: %d.%d,
> profile:
> > > %d, level: %d, "
> > > > -           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility
> id:
> > > %d\n",
> > > > -           dovi->dv_profile > 10 ? "dvwC" : (dovi->dv_profile > 7 ?
> > > "dvvC" : "dvcC"),
> > > > -           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);
> > > > +
> > > > +    size = ff_isom_put_dvcc_dvvc(s, buf, sizeof(buf), dovi);
> > >
> > > I think you need check the return of ff_isom_put_dvcc_dvvc().
> > >
> > >
> > Wouldn't it necessarily return the constant ISOM_DVCC_DVVC_SIZE in this
> > case?
>
> yes, now it'll return negative error code in one error case. Why the out
> parameter is
> using array instead of pointer, as the size of buffer is one of parameters
> also.
> So if it's array, why it's necessary to check the size?
>

External users of the function can pass either an array or pointer.
The size addition was suggested here:
https://ffmpeg.org/pipermail/ffmpeg-devel/2021-September/285877.html

So in the case a pointer is used, the size is necessary.

> Since the only error condition is if the passed size is lower than that,
> > but the buffer is constant sized.
> >
> > And the PutBitContext, being flushed, would return the same value.
> >
> >
> > > > +
> > > > +    avio_write(pb, buf, size);
> > > > +
> > > >      return 32; /* 8 + 24 */
> > > >  }
> > > >
> > > > --
> > > > 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".
> > >
> > _______________________________________________
> > 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".
>
quietvoid Dec. 4, 2021, 1:55 a.m. UTC | #5 
On Fri, Dec 3, 2021 at 8:51 PM quietvoid <tcchlisop0@gmail.com> wrote:

>
> On Fri, Dec 3, 2021 at 8:40 PM <lance.lmwang@gmail.com> wrote:
>
>> On Fri, Dec 03, 2021 at 08:27:06PM -0500, quietvoid wrote:
>> > On Fri, Dec 3, 2021 at 8:19 PM <lance.lmwang@gmail.com> wrote:
>> >
>> > > On Sat, Dec 04, 2021 at 02:09:04AM +0100, quietvoid wrote:
>> > > > From: quietvoid <tcchlisop0@gmail.com>
>> > > >
>> > > > Improves code legibility by not using bit shifts.
>> > > > Also avoids duplicating the dvcC/dvvC ISOM box writing code.
>> > > >
>> > > > Signed-off-by: quietvoid <tcChlisop0@gmail.com>
>> > > > ---
>> > > >  libavformat/movenc.c | 26 +++++++++-----------------
>> > > >  1 file changed, 9 insertions(+), 17 deletions(-)
>> > > >
>> > > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>> > > > index 38ff90833a..79f7d70747 100644
>> > > > --- a/libavformat/movenc.c
>> > > > +++ b/libavformat/movenc.c
>> > > > @@ -27,6 +27,7 @@
>> > > >  #include "movenc.h"
>> > > >  #include "avformat.h"
>> > > >  #include "avio_internal.h"
>> > > > +#include "dovi_isom.h"
>> > > >  #include "riff.h"
>> > > >  #include "avio.h"
>> > > >  #include "isom.h"
>> > > > @@ -1911,6 +1912,9 @@ static int mov_write_sv3d_tag(AVFormatContext
>> *s,
>> > > AVIOContext *pb, AVSphericalMa
>> > > >
>> > > >  static int mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext
>> *pb,
>> > > AVDOVIDecoderConfigurationRecord *dovi)
>> > > >  {
>> > > > +    uint8_t buf[ISOM_DVCC_DVVC_SIZE];
>> > > > +    int size;
>> > > > +
>> > > >      avio_wb32(pb, 32); /* size = 8 + 24 */
>> > > >      if (dovi->dv_profile > 10)
>> > > >          ffio_wfourcc(pb, "dvwC");
>> > > > @@ -1918,23 +1922,11 @@ static int
>> > > mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb, AVDOVIDe
>> > > >          ffio_wfourcc(pb, "dvvC");
>> > > >      else
>> > > >          ffio_wfourcc(pb, "dvcC");
>> > > > -    avio_w8(pb, dovi->dv_version_major);
>> > > > -    avio_w8(pb, dovi->dv_version_minor);
>> > > > -    avio_wb16(pb, (dovi->dv_profile << 9) | (dovi->dv_level << 3) |
>> > > > -              (dovi->rpu_present_flag << 2) |
>> (dovi->el_present_flag <<
>> > > 1) |
>> > > > -              dovi->bl_present_flag);
>> > > > -    avio_wb32(pb, (dovi->dv_bl_signal_compatibility_id << 28) | 0);
>> > > > -
>> > > > -    ffio_fill(pb, 0, 4 * 4); /* reserved */
>> > > > -    av_log(s, AV_LOG_DEBUG, "DOVI in %s box, version: %d.%d,
>> profile:
>> > > %d, level: %d, "
>> > > > -           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility
>> id:
>> > > %d\n",
>> > > > -           dovi->dv_profile > 10 ? "dvwC" : (dovi->dv_profile > 7 ?
>> > > "dvvC" : "dvcC"),
>> > > > -           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);
>> > > > +
>> > > > +    size = ff_isom_put_dvcc_dvvc(s, buf, sizeof(buf), dovi);
>> > >
>> > > I think you need check the return of ff_isom_put_dvcc_dvvc().
>> > >
>> > >
>> > Wouldn't it necessarily return the constant ISOM_DVCC_DVVC_SIZE in this
>> > case?
>>
>> yes, now it'll return negative error code in one error case. Why the out
>> parameter is
>> using array instead of pointer, as the size of buffer is one of
>> parameters also.
>> So if it's array, why it's necessary to check the size?
>>
>
> External users of the function can pass either an array or pointer.
>
Actually this is not true, I was looking at the wrong code.
I don't think the size is necessary, then.

The size addition was suggested here:
> https://ffmpeg.org/pipermail/ffmpeg-devel/2021-September/285877.html
>
> So in the case a pointer is used, the size is necessary.
>
> > Since the only error condition is if the passed size is lower than that,
>> > but the buffer is constant sized.
>> >
>> > And the PutBitContext, being flushed, would return the same value.
>> >
>> >
>> > > > +
>> > > > +    avio_write(pb, buf, size);
>> > > > +
>> > > >      return 32; /* 8 + 24 */
>> > > >  }
>> > > >
>> > > > --
>> > > > 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".
>> > >
>> > _______________________________________________
>> > 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".
>>
>
Lance Wang Dec. 4, 2021, 2:21 a.m. UTC | #6 
On Fri, Dec 03, 2021 at 08:55:08PM -0500, quietvoid wrote:
> On Fri, Dec 3, 2021 at 8:51 PM quietvoid <tcchlisop0@gmail.com> wrote:
> 
> >
> > On Fri, Dec 3, 2021 at 8:40 PM <lance.lmwang@gmail.com> wrote:
> >
> >> On Fri, Dec 03, 2021 at 08:27:06PM -0500, quietvoid wrote:
> >> > On Fri, Dec 3, 2021 at 8:19 PM <lance.lmwang@gmail.com> wrote:
> >> >
> >> > > On Sat, Dec 04, 2021 at 02:09:04AM +0100, quietvoid wrote:
> >> > > > From: quietvoid <tcchlisop0@gmail.com>
> >> > > >
> >> > > > Improves code legibility by not using bit shifts.
> >> > > > Also avoids duplicating the dvcC/dvvC ISOM box writing code.
> >> > > >
> >> > > > Signed-off-by: quietvoid <tcChlisop0@gmail.com>
> >> > > > ---
> >> > > >  libavformat/movenc.c | 26 +++++++++-----------------
> >> > > >  1 file changed, 9 insertions(+), 17 deletions(-)
> >> > > >
> >> > > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> >> > > > index 38ff90833a..79f7d70747 100644
> >> > > > --- a/libavformat/movenc.c
> >> > > > +++ b/libavformat/movenc.c
> >> > > > @@ -27,6 +27,7 @@
> >> > > >  #include "movenc.h"
> >> > > >  #include "avformat.h"
> >> > > >  #include "avio_internal.h"
> >> > > > +#include "dovi_isom.h"
> >> > > >  #include "riff.h"
> >> > > >  #include "avio.h"
> >> > > >  #include "isom.h"
> >> > > > @@ -1911,6 +1912,9 @@ static int mov_write_sv3d_tag(AVFormatContext
> >> *s,
> >> > > AVIOContext *pb, AVSphericalMa
> >> > > >
> >> > > >  static int mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext
> >> *pb,
> >> > > AVDOVIDecoderConfigurationRecord *dovi)
> >> > > >  {
> >> > > > +    uint8_t buf[ISOM_DVCC_DVVC_SIZE];
> >> > > > +    int size;
> >> > > > +
> >> > > >      avio_wb32(pb, 32); /* size = 8 + 24 */
> >> > > >      if (dovi->dv_profile > 10)
> >> > > >          ffio_wfourcc(pb, "dvwC");
> >> > > > @@ -1918,23 +1922,11 @@ static int
> >> > > mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb, AVDOVIDe
> >> > > >          ffio_wfourcc(pb, "dvvC");
> >> > > >      else
> >> > > >          ffio_wfourcc(pb, "dvcC");
> >> > > > -    avio_w8(pb, dovi->dv_version_major);
> >> > > > -    avio_w8(pb, dovi->dv_version_minor);
> >> > > > -    avio_wb16(pb, (dovi->dv_profile << 9) | (dovi->dv_level << 3) |
> >> > > > -              (dovi->rpu_present_flag << 2) |
> >> (dovi->el_present_flag <<
> >> > > 1) |
> >> > > > -              dovi->bl_present_flag);
> >> > > > -    avio_wb32(pb, (dovi->dv_bl_signal_compatibility_id << 28) | 0);
> >> > > > -
> >> > > > -    ffio_fill(pb, 0, 4 * 4); /* reserved */
> >> > > > -    av_log(s, AV_LOG_DEBUG, "DOVI in %s box, version: %d.%d,
> >> profile:
> >> > > %d, level: %d, "
> >> > > > -           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility
> >> id:
> >> > > %d\n",
> >> > > > -           dovi->dv_profile > 10 ? "dvwC" : (dovi->dv_profile > 7 ?
> >> > > "dvvC" : "dvcC"),
> >> > > > -           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);
> >> > > > +
> >> > > > +    size = ff_isom_put_dvcc_dvvc(s, buf, sizeof(buf), dovi);
> >> > >
> >> > > I think you need check the return of ff_isom_put_dvcc_dvvc().
> >> > >
> >> > >
> >> > Wouldn't it necessarily return the constant ISOM_DVCC_DVVC_SIZE in this
> >> > case?
> >>
> >> yes, now it'll return negative error code in one error case. Why the out
> >> parameter is
> >> using array instead of pointer, as the size of buffer is one of
> >> parameters also.
> >> So if it's array, why it's necessary to check the size?
> >>
> >
> > External users of the function can pass either an array or pointer.
> >
> Actually this is not true, I was looking at the wrong code.
> I don't think the size is necessary, then.
> 
> The size addition was suggested here:
> > https://ffmpeg.org/pipermail/ffmpeg-devel/2021-September/285877.html

As suggested, for the array is fixed size already, so the size of array can
be removed and don't need check the size anymore.

> >
> > So in the case a pointer is used, the size is necessary.
> >
> > > Since the only error condition is if the passed size is lower than that,
> >> > but the buffer is constant sized.
> >> >
> >> > And the PutBitContext, being flushed, would return the same value.
> >> >
> >> >
> >> > > > +
> >> > > > +    avio_write(pb, buf, size);
> >> > > > +
> >> > > >      return 32; /* 8 + 24 */
> >> > > >  }
> >> > > >
> >> > > > --
> >> > > > 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".
> >> > >
> >> > _______________________________________________
> >> > 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".
> >>
> >
> _______________________________________________
> 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".
Andreas Rheinhardt Dec. 5, 2021, 2:49 p.m. UTC | #7 
quietvoid:
> On Fri, Dec 3, 2021 at 8:40 PM <lance.lmwang@gmail.com> wrote:
> 
>> On Fri, Dec 03, 2021 at 08:27:06PM -0500, quietvoid wrote:
>>> On Fri, Dec 3, 2021 at 8:19 PM <lance.lmwang@gmail.com> wrote:
>>>
>>>> On Sat, Dec 04, 2021 at 02:09:04AM +0100, quietvoid wrote:
>>>>> From: quietvoid <tcchlisop0@gmail.com>
>>>>>
>>>>> Improves code legibility by not using bit shifts.
>>>>> Also avoids duplicating the dvcC/dvvC ISOM box writing code.
>>>>>
>>>>> Signed-off-by: quietvoid <tcChlisop0@gmail.com>
>>>>> ---
>>>>>  libavformat/movenc.c | 26 +++++++++-----------------
>>>>>  1 file changed, 9 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>>>> index 38ff90833a..79f7d70747 100644
>>>>> --- a/libavformat/movenc.c
>>>>> +++ b/libavformat/movenc.c
>>>>> @@ -27,6 +27,7 @@
>>>>>  #include "movenc.h"
>>>>>  #include "avformat.h"
>>>>>  #include "avio_internal.h"
>>>>> +#include "dovi_isom.h"
>>>>>  #include "riff.h"
>>>>>  #include "avio.h"
>>>>>  #include "isom.h"
>>>>> @@ -1911,6 +1912,9 @@ static int mov_write_sv3d_tag(AVFormatContext
>> *s,
>>>> AVIOContext *pb, AVSphericalMa
>>>>>
>>>>>  static int mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext
>> *pb,
>>>> AVDOVIDecoderConfigurationRecord *dovi)
>>>>>  {
>>>>> +    uint8_t buf[ISOM_DVCC_DVVC_SIZE];
>>>>> +    int size;
>>>>> +
>>>>>      avio_wb32(pb, 32); /* size = 8 + 24 */
>>>>>      if (dovi->dv_profile > 10)
>>>>>          ffio_wfourcc(pb, "dvwC");
>>>>> @@ -1918,23 +1922,11 @@ static int
>>>> mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb, AVDOVIDe
>>>>>          ffio_wfourcc(pb, "dvvC");
>>>>>      else
>>>>>          ffio_wfourcc(pb, "dvcC");
>>>>> -    avio_w8(pb, dovi->dv_version_major);
>>>>> -    avio_w8(pb, dovi->dv_version_minor);
>>>>> -    avio_wb16(pb, (dovi->dv_profile << 9) | (dovi->dv_level << 3) |
>>>>> -              (dovi->rpu_present_flag << 2) |
>> (dovi->el_present_flag <<
>>>> 1) |
>>>>> -              dovi->bl_present_flag);
>>>>> -    avio_wb32(pb, (dovi->dv_bl_signal_compatibility_id << 28) | 0);
>>>>> -
>>>>> -    ffio_fill(pb, 0, 4 * 4); /* reserved */
>>>>> -    av_log(s, AV_LOG_DEBUG, "DOVI in %s box, version: %d.%d,
>> profile:
>>>> %d, level: %d, "
>>>>> -           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility
>> id:
>>>> %d\n",
>>>>> -           dovi->dv_profile > 10 ? "dvwC" : (dovi->dv_profile > 7 ?
>>>> "dvvC" : "dvcC"),
>>>>> -           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);
>>>>> +
>>>>> +    size = ff_isom_put_dvcc_dvvc(s, buf, sizeof(buf), dovi);
>>>>
>>>> I think you need check the return of ff_isom_put_dvcc_dvvc().
>>>>
>>>>
>>> Wouldn't it necessarily return the constant ISOM_DVCC_DVVC_SIZE in this
>>> case?
>>
>> yes, now it'll return negative error code in one error case. Why the out
>> parameter is
>> using array instead of pointer, as the size of buffer is one of parameters
>> also.
>> So if it's array, why it's necessary to check the size?
>>
> 
> External users of the function can pass either an array or pointer.

This function is libavformat-only; there are no external users.

> The size addition was suggested here:
> https://ffmpeg.org/pipermail/ffmpeg-devel/2021-September/285877.html

I actually suggested this to you so that you can remove the check from
the function.

> 
> So in the case a pointer is used, the size is necessary.
> 
>> Since the only error condition is if the passed size is lower than that,
>>> but the buffer is constant sized.
>>>
>>> And the PutBitContext, being flushed, would return the same value.
>>>
>>>
>>>>> +
>>>>> +    avio_write(pb, buf, size);
>>>>> +
>>>>>      return 32; /* 8 + 24 */
>>>>>  }
>>>>>
>>>>> --
>>>>> 2.34.1
diff mbox series

Patch

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 38ff90833a..79f7d70747 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -27,6 +27,7 @@ 
 #include "movenc.h"
 #include "avformat.h"
 #include "avio_internal.h"
+#include "dovi_isom.h"
 #include "riff.h"
 #include "avio.h"
 #include "isom.h"
@@ -1911,6 +1912,9 @@  static int mov_write_sv3d_tag(AVFormatContext *s, AVIOContext *pb, AVSphericalMa
 
 static int mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb, AVDOVIDecoderConfigurationRecord *dovi)
 {
+    uint8_t buf[ISOM_DVCC_DVVC_SIZE];
+    int size;
+
     avio_wb32(pb, 32); /* size = 8 + 24 */
     if (dovi->dv_profile > 10)
         ffio_wfourcc(pb, "dvwC");
@@ -1918,23 +1922,11 @@  static int mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb, AVDOVIDe
         ffio_wfourcc(pb, "dvvC");
     else
         ffio_wfourcc(pb, "dvcC");
-    avio_w8(pb, dovi->dv_version_major);
-    avio_w8(pb, dovi->dv_version_minor);
-    avio_wb16(pb, (dovi->dv_profile << 9) | (dovi->dv_level << 3) |
-              (dovi->rpu_present_flag << 2) | (dovi->el_present_flag << 1) |
-              dovi->bl_present_flag);
-    avio_wb32(pb, (dovi->dv_bl_signal_compatibility_id << 28) | 0);
-
-    ffio_fill(pb, 0, 4 * 4); /* reserved */
-    av_log(s, AV_LOG_DEBUG, "DOVI in %s box, version: %d.%d, profile: %d, level: %d, "
-           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility id: %d\n",
-           dovi->dv_profile > 10 ? "dvwC" : (dovi->dv_profile > 7 ? "dvvC" : "dvcC"),
-           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);
+
+    size = ff_isom_put_dvcc_dvvc(s, buf, sizeof(buf), dovi);
+
+    avio_write(pb, buf, size);
+
     return 32; /* 8 + 24 */
 }