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

[quietvoid]

On Fri, Dec 3, 2021 at 8:51 PM quietvoid <tcchlisop0 at gmail.com> wrote:

>
> On Fri, Dec 3, 2021 at 8:40 PM <lance.lmwang at 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 at gmail.com> wrote:
>> >
>> > > On Sat, Dec 04, 2021 at 02:09:04AM +0100, quietvoid wrote:
>> > > > From: quietvoid <tcchlisop0 at 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 at 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 at ffmpeg.org
>> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > > >
>> > > > To unsubscribe, visit link above, or email
>> > > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>> > >
>> > > --
>> > > Thanks,
>> > > Limin Wang
>> > > _______________________________________________
>> > > ffmpeg-devel mailing list
>> > > ffmpeg-devel at ffmpeg.org
>> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > >
>> > > To unsubscribe, visit link above, or email
>> > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>> > >
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel at ffmpeg.org
>> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>> > To unsubscribe, visit link above, or email
>> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
>> --
>> Thanks,
>> Limin Wang
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
>