diff mbox

[FFmpeg-devel] Adding mkdir option for img2enc (2nd attempt)

Message ID 9483770e-4b2f-94ef-e18e-dfe358a55420@escribe.co.uk
State New
Headers show

Commit Message

Dr Alan Barclay Dec. 27, 2017, 12:27 p.m. UTC 
Hi,

Resending the two (git format-patch) patches, without the top lines 
removed (which I thought I needed to do as some patch emails didn't seem 
to have them).

All comments and help appreciated.

Thanks and Regards,
Alan.


On 27/12/17 01:41, Michael Niedermayer wrote:
> On Tue, Dec 26, 2017 at 10:44:31PM +0000, Dr Alan Barclay wrote:
>> Hi All,
>>
>> Please would someone with an interest in img2enc take a look at my revised
>> patches for a minor feature addition and consider committing it to the main
>> line for me.
>>
>> Example:
>> ffmpeg -i ~/trailer.mp4 -strftime 1 -mkdir 1 %Y/%m/%d/out_%H-%M-%S.jpg
>>
>> Without the new mkdir option, this command will fail if the directory
>> hierarchy for the jpg files does not already exist, which can be difficult
>> to predict for time-stamped directories.
>>
>> This patch adds a mkdir option to img2enc which invites it to make whatever
>> directory hierarchy is necessary for each output file. When used in
>> conjunction with the strftime then the jpg files will be located in a newly
>> created (time-stamped) directory as processing progresses.
>>
>> My typical usage scenario is capturing a long-running live video feed
>> (perhaps time-lapsed) and storing the resulting images in a time-stamped
>> directory hierarchy fashion, rather than as a numbered sequence of files in
>> a single directory.
>>
>> If you look at the code you will see that only a half dozen lines of code
>> were required in img2enc. The function for creating directories already
>> existed in hlsenc.c but I've moved into utils.c as I presumed that was a
>> more generic location for it.
>>
>> All comments appreciated.
>>
>> Thanks ad Regards,
>> Alan.
>>
>>
>> On 17/12/17 22:46, Carl Eugen Hoyos wrote:
>>> 2017-12-17 23:41 GMT+01:00 Dr Alan Barclay <alan@escribe.co.uk>:
>>>
>>>> Please would someone with an interest in img2enc take a look
>>>> at my minor feature addition and consider committing it to the
>>>> main line for me.
>>> To be acceptable, the patch has to be split in two and please
>>> move the definition into internal.h
>>>
>>> Carl Eugen
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>   hlsenc.c   |   35 +----------------------------------
>>   internal.h |    7 +++++++
>>   utils.c    |   33 +++++++++++++++++++++++++++++++++
>>   3 files changed, 41 insertions(+), 34 deletions(-)
>> 9560fd03958f79f77b01c0e02c55d98e3dc7b937  0001-Move-mkdir_p-renamed-ff_mkdir_p-from-hlsenc.c-to-uti.patch
>> ---
>>   libavformat/hlsenc.c   | 35 +----------------------------------
>>   libavformat/internal.h |  7 +++++++
>>   libavformat/utils.c    | 33 +++++++++++++++++++++++++++++++++
>>   3 files changed, 41 insertions(+), 34 deletions(-)
> these patches are missing commit messages
> patches should be genrated with git format-patch or git send-email
>
>
> [...]
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Comments

Derek Buitenhuis Dec. 27, 2017, 4:01 p.m. UTC | #1 
Hi,

On 12/27/2017 12:27 PM, Dr Alan Barclay wrote:
> Resending the two (git format-patch) patches, without the top lines 
> removed (which I thought I needed to do as some patch emails didn't seem 
> to have them).
> 
> All comments and help appreciated.

[...]

> Subject: [PATCH 1/2] Move mkdir_p (renamed ff_mkdir_p) from hlsenc.c to
>  utils.c.
> 
> ---
>  libavformat/hlsenc.c   | 35 +----------------------------------
>  libavformat/internal.h |  7 +++++++
>  libavformat/utils.c    | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 34 deletions(-)

On a technical level, this patch looks OK.

> Subject: [PATCH 2/2] Adding mkdir option for img2enc.
> 
> ---
>  libavformat/img2enc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

I'm not sure how others feel about the premise (mkdir in img2enc).
I personally don't mind - though, maybe it should be default instead
of an option? (Maybe a bad idea.)

> +        if (img->use_mkdir) {
> +            char *temp_filename = av_strdup(filename);
> +            const char *temp_path = av_dirname(temp_filename);
> +            ff_mkdir_p(temp_path);
> +            av_free(temp_filename);
> +        }

This lacks error checks for av_strdup and ff_mkdir_p.

- Derek
diff mbox

Patch

From 0f8a3b00c502bdadd4571c3966e39aa00da17fc3 Mon Sep 17 00:00:00 2001
From: "Dr. Alan Barclay" <alan@escribe.co.uk>
Date: Tue, 26 Dec 2017 22:37:15 +0000
Subject: [PATCH 2/2] Adding mkdir option for img2enc.

---
 libavformat/img2enc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/libavformat/img2enc.c b/libavformat/img2enc.c
index b680676bff..a8ee064396 100644
--- a/libavformat/img2enc.c
+++ b/libavformat/img2enc.c
@@ -42,6 +42,7 @@  typedef struct VideoMuxData {
     char target[4][1024];
     int update;
     int use_strftime;
+    int use_mkdir;
     int frame_pts;
     const char *muxer;
     int use_rename;
@@ -114,6 +115,12 @@  static int write_packet(AVFormatContext *s, AVPacket *pkt)
                    img->img_number, img->path);
             return AVERROR(EINVAL);
         }
+        if (img->use_mkdir) {
+            char *temp_filename = av_strdup(filename);
+            const char *temp_path = av_dirname(temp_filename);
+            ff_mkdir_p(temp_path);
+            av_free(temp_filename);
+        }
         for (i = 0; i < 4; i++) {
             snprintf(img->tmp[i], sizeof(img->tmp[i]), "%s.tmp", filename);
             av_strlcpy(img->target[i], filename, sizeof(img->target[i]));
@@ -212,6 +219,7 @@  static const AVOption muxoptions[] = {
     { "update",       "continuously overwrite one file", OFFSET(update),  AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0,       1, ENC },
     { "start_number", "set first number in the sequence", OFFSET(img_number), AV_OPT_TYPE_INT,  { .i64 = 1 }, 0, INT_MAX, ENC },
     { "strftime",     "use strftime for filename", OFFSET(use_strftime),  AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, ENC },
+    { "mkdir",        "make sub-dirs as required", OFFSET(use_mkdir),  AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, ENC },
     { "frame_pts",    "use current frame pts for filename", OFFSET(frame_pts),  AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, ENC },
     { "atomic_writing", "write files atomically (using temporary files and renames)", OFFSET(use_rename), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, ENC },
     { NULL },
-- 
2.11.0