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

[Andreas Rheinhardt]

quietvoid:
> 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.

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