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

[James Almer]

On 8/25/2018 10:30 AM, Marton Balint wrote:
> 
> 
> 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.

No, adding a function that will be obsolete from day 1 is not a good
idea. Adding API is simple, but removing it takes years. So lets instead
add one using AVBPrintf that will be definite.

Also not called whatever3 while at it, if possible.

> 
> Thanks,
> Marton