[FFmpeg-devel] [PATCH] Support 64-bit integers for av_get_frame_filename2()

[Marton Balint]

On Sat, 25 Aug 2018, Michael Niedermayer wrote:

> On Fri, Aug 24, 2018 at 04:33:18PM -0300, James Almer wrote:
>> On 8/24/2018 3:56 PM, Devin Heitmueller wrote:
>>> Create a new av_get_frame_filename3() API which is just like the
>>> previous version but accepts a 64-bit integer for the "number"
>>> argument.  This is useful in cases where you want to put the
>>> original PTS into the filename (which can be larger than 32-bits).
>>>
>>> Tested with:
>>>
>>> ./ffmpeg -copyts -vsync 0 -i foo.ts -frame_pts 1 -enc_time_base -1 foo_%d.png
>>>
>>> Signed-off-by: Devin Heitmueller <dheitmueller at ltnglobal.com>
>>> ---
>>>  libavformat/avformat.h | 2 ++
>>>  libavformat/img2enc.c  | 2 +-
>>>  libavformat/utils.c    | 9 +++++++--
>>>  3 files changed, 10 insertions(+), 3 deletions(-)
>>
>> Missing APIChanges entry and libavformat minor version bump.
>>
>>>
>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>> index fdaffa5bf4..c358a9a71e 100644
>>> --- a/libavformat/avformat.h
>>> +++ b/libavformat/avformat.h
>>> @@ -2896,6 +2896,8 @@ void av_dump_format(AVFormatContext *ic,
>>>   * @param flags AV_FRAME_FILENAME_FLAGS_*
>>>   * @return 0 if OK, -1 on format error
>>>   */
>>> +int av_get_frame_filename3(char *buf, int buf_size,
>>> +                          const char *path, int64_t number, int flags);
>>
>> Make buf_size of size_t type while at it.
>>
>>>  int av_get_frame_filename2(char *buf, int buf_size,
>>>                            const char *path, int number, int flags);
>>>
>>> diff --git a/libavformat/img2enc.c b/libavformat/img2enc.c
>>> index a09cc8ec50..414eb827e2 100644
>>> --- a/libavformat/img2enc.c
>>> +++ b/libavformat/img2enc.c
>>> @@ -101,7 +101,7 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt)
>>>                  return AVERROR(EINVAL);
>>>              }
>>>          } else if (img->frame_pts) {
>>> -            if (av_get_frame_filename2(filename, sizeof(filename), img->path, pkt->pts, AV_FRAME_FILENAME_FLAGS_MULTIPLE) < 0) {
>>> +            if (av_get_frame_filename3(filename, sizeof(filename), img->path, pkt->pts, AV_FRAME_FILENAME_FLAGS_MULTIPLE) < 0) {
>>>                  av_log(s, AV_LOG_ERROR, "Cannot write filename by pts of the frames.");
>>>                  return AVERROR(EINVAL);
>>>              }
>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>> index b0b5e164a6..d9d4d38a44 100644
>>> --- a/libavformat/utils.c
>>> +++ b/libavformat/utils.c
>>> @@ -4666,7 +4666,7 @@ uint64_t ff_get_formatted_ntp_time(uint64_t ntp_time_us)
>>>      return ntp_ts;
>>>  }
>>>
>>> -int av_get_frame_filename2(char *buf, int buf_size, const char *path, int number, int flags)
>>> +int av_get_frame_filename3(char *buf, int buf_size, const char *path, int64_t number, int flags)
>>>  {
>>>      const char *p;
>>>      char *q, buf1[20], c;
>>> @@ -4696,7 +4696,7 @@ int av_get_frame_filename2(char *buf, int buf_size, const char *path, int number
>>>                  percentd_found = 1;
>>>                  if (number < 0)
>>>                      nd += 1;
>>> -                snprintf(buf1, sizeof(buf1), "%0*d", nd, number);
>>> +                snprintf(buf1, sizeof(buf1), "%0*" PRId64, nd, number);
>>
>> SCNd64.
>>
>>>                  len = strlen(buf1);
>>>                  if ((q - buf + len) > buf_size - 1)
>>>                      goto fail;
>>> @@ -4721,6 +4721,11 @@ fail:
>>>      return -1;
>>>  }
>>>
>>> +int av_get_frame_filename2(char *buf, int buf_size, const char *path, int number, int flags)
>>> +{
>>> +    return av_get_frame_filename3(buf, buf_size, path, number, flags);
>>> +}
>>> +
>>>  int av_get_frame_filename(char *buf, int buf_size, const char *path, int number)
>>>  {
>>>      return av_get_frame_filename2(buf, buf_size, path, number, 0);
>>>
>>
>> No opinion on the addition, so wait for someone else to review as well.
>
> i think it makes sense
>
> also the old function should be deprecated. I think theres no reason to
> use the old one if the 64bit one is added. We could refrain from removing
> the old though for longer as it has no risk of breaking. But deprecation
> would notify users that we have something newer/better to replace it

Well, actually av_get_frame_filename3 should be deprecated too soon, 
because users should not use these functions because they limit the file 
path to a fixed length. An AVBPrintf based replacement should be used 
instead in the long run.

On the other hand that is a bit more work, so I am fine with the patch as 
is.

Thanks,
Marton