Re: [FFmpeg-devel] [PATCH v2 1/2] avformat/file: Add a specialized url_check callback for pipe protocol
Bump.
Is anyone interested in looking at these two patches? I’m trying to get
into ffmpeg development and it would be nice to get some experience of
the full process of getting a patch landed. I don’t have commit rights.
Thanks in advance for any feedback.
Regards,
– Neil
Neil Roberts writes:
> Using file_check for the pipe protocol doesn't make sense. For example,
> for a URL like “pipe:0” it would end up checking whether the “pipe:0”
> file exists. This patch instead makes it check the access modes on the
> corresponding file descriptor using fcntl on *nix and DuplicateHandle on
> Windows.
>
> v2: Use DuplicateHandle on Windows to check whether the duplicated
> handle can have the corresponding access flags.
>
> Signed-off-by: Neil Roberts
> ---
> libavformat/file.c | 74 --
> 1 file changed, 71 insertions(+), 3 deletions(-)
>
> diff --git a/libavformat/file.c b/libavformat/file.c
> index 98c9e81bcb..b3f7bc9282 100644
> --- a/libavformat/file.c
> +++ b/libavformat/file.c
> @@ -375,9 +375,8 @@ const URLProtocol ff_file_protocol = {
>
> #if CONFIG_PIPE_PROTOCOL
>
> -static int pipe_open(URLContext *h, const char *filename, int flags)
> +static int pipe_get_fd(const char *filename, int flags)
> {
> -FileContext *c = h->priv_data;
> int fd;
> char *final;
> av_strstart(filename, "pipe:", );
> @@ -390,6 +389,14 @@ static int pipe_open(URLContext *h, const char
> *filename, int flags)
> fd = 0;
> }
> }
> +
> +return fd;
> +}
> +
> +static int pipe_open(URLContext *h, const char *filename, int flags)
> +{
> +FileContext *c = h->priv_data;
> +int fd = pipe_get_fd(filename, flags);
> #if HAVE_SETMODE
> setmode(fd, O_BINARY);
> #endif
> @@ -398,13 +405,74 @@ static int pipe_open(URLContext *h, const char
> *filename, int flags)
> return 0;
> }
>
> +static int pipe_check(URLContext *h, int mask)
> +{
> +int fd = pipe_get_fd(h->filename, mask);
> +int access = 0;
> +
> +#ifdef _WIN32
> +
> +HANDLE pipe_handle = (HANDLE) _get_osfhandle(fd);
> +HANDLE tmp_handle;
> +
> +if (pipe_handle == INVALID_HANDLE_VALUE)
> +return AVERROR(errno);
> +
> +if (mask & AVIO_FLAG_READ &&
> +DuplicateHandle(GetCurrentProcess(), /* hSourceProcessHandle */
> +pipe_handle,
> +GetCurrentProcess(), /* hTargetProcessHandle */
> +_handle,
> +FILE_READ_DATA,
> +FALSE, /* bInheritHandle */
> +0 /* options */)) {
> +access |= AVIO_FLAG_READ;
> +CloseHandle(tmp_handle);
> +}
> +
> +if (mask & AVIO_FLAG_WRITE &&
> +DuplicateHandle(GetCurrentProcess(), /* hSourceProcessHandle */
> +pipe_handle,
> +GetCurrentProcess(), /* hTargetProcessHandle */
> +_handle,
> +FILE_WRITE_DATA,
> +FALSE, /* bInheritHandle */
> +0 /* options */)) {
> +access |= AVIO_FLAG_WRITE;
> +CloseHandle(tmp_handle);
> +}
> +
> +#else /* _WIN32 */
> +
> +int status_flags = fcntl(fd, F_GETFL);
> +
> +if (status_flags == -1)
> +return AVERROR(errno);
> +
> +switch (status_flags & O_ACCMODE) {
> +case O_RDONLY:
> +access = AVIO_FLAG_READ;
> +break;
> +case O_WRONLY:
> +access = AVIO_FLAG_WRITE;
> +break;
> +case O_RDWR:
> +access = AVIO_FLAG_READ | AVIO_FLAG_WRITE;
> +break;
> +}
> +
> +#endif /* _WIN32 */
> +
> +return access & mask;
> +}
> +
> const URLProtocol ff_pipe_protocol = {
> .name= "pipe",
> .url_open= pipe_open,
> .url_read= file_read,
> .url_write = file_write,
> .url_get_file_handle = file_get_handle,
> -.url_check = file_check,
> +.url_check = pipe_check,
> .priv_data_size = sizeof(FileContext),
> .priv_data_class = _class,
> .default_whitelist = "crypto,data"
> --
> 2.37.2
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v2 2/2] avformat/tests: Add a test for avio_check with the pipe protocol
Creates a UNIX pipe and then verifies that avio_check returns the right
access flags for the two ends of the pipe.
v2: Add support for the Windows version of _pipe
Signed-off-by: Neil Roberts
---
libavformat/Makefile | 1 +
libavformat/tests/.gitignore | 1 +
libavformat/tests/pipe.c | 108 +++
tests/fate/libavformat.mak | 5 ++
4 files changed, 115 insertions(+)
create mode 100644 libavformat/tests/pipe.c
diff --git a/libavformat/Makefile b/libavformat/Makefile
index f67a99f839..9c681c58c5 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -732,6 +732,7 @@ TESTPROGS-$(CONFIG_MOV_MUXER)+= movenc
TESTPROGS-$(CONFIG_NETWORK) += noproxy
TESTPROGS-$(CONFIG_SRTP) += srtp
TESTPROGS-$(CONFIG_IMF_DEMUXER) += imf
+TESTPROGS-$(CONFIG_PIPE_PROTOCOL)+= pipe
TOOLS = aviocat \
ismindex\
diff --git a/libavformat/tests/.gitignore b/libavformat/tests/.gitignore
index cdd0cce061..567d6f9e40 100644
--- a/libavformat/tests/.gitignore
+++ b/libavformat/tests/.gitignore
@@ -7,3 +7,4 @@
/srtp
/url
/seek_utils
+/pipe
diff --git a/libavformat/tests/pipe.c b/libavformat/tests/pipe.c
new file mode 100644
index 00..18a8551fd5
--- /dev/null
+++ b/libavformat/tests/pipe.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright (c) 2022 Neil Roberts
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include "libavformat/avio.h"
+#include "libavutil/error.h"
+
+static int check_pipe(const char *url, int mask, int expected)
+{
+int flags = avio_check(url, mask);
+
+if (flags < 0) {
+fprintf(stderr,
+"avio_check for %s with mask 0x%x failed: %s\n",
+url,
+mask,
+av_err2str(flags));
+return 0;
+}
+
+if (flags != expected) {
+fprintf(stderr,
+"Wrong result returned from avio_check for %s with mask 0x%x. "
+"Expected 0x%x but received 0x%x\n",
+url,
+mask,
+expected,
+flags);
+return 0;
+}
+
+return 1;
+}
+
+int main(int argc, char **argv)
+{
+int ret = 0;
+int pipe_fds[2];
+char read_url[20], write_url[20];
+int check_invalid_ret;
+int pipe_ret;
+
+#ifdef _WIN32
+pipe_ret = _pipe(pipe_fds, 1024 /* psize */, 0 /* textmode */);
+#else
+pipe_ret = pipe(pipe_fds);
+#endif
+
+if (pipe_ret == -1) {
+fprintf(stderr, "error creating pipe: %s\n", strerror(errno));
+return 1;
+}
+
+snprintf(read_url, sizeof(read_url), "pipe:%d", pipe_fds[0]);
+snprintf(write_url, sizeof(write_url), "pipe:%d", pipe_fds[1]);
+
+if (!check_pipe(read_url,
+AVIO_FLAG_READ | AVIO_FLAG_WRITE,
+AVIO_FLAG_READ))
+ret = 1;
+
+if (!check_pipe(write_url,
+AVIO_FLAG_READ | AVIO_FLAG_WRITE,
+AVIO_FLAG_WRITE))
+ret = 1;
+
+/* Ensure that we don't get flags that we didn't ask for */
+if (!check_pipe(read_url, AVIO_FLAG_WRITE, 0))
+ret = 1;
+
+close(pipe_fds[0]);
+close(pipe_fds[1]);
+
+/* An invalid fd should return EBADF */
+check_invalid_ret = avio_check(read_url, AVIO_FLAG_READ);
+
+if (check_invalid_ret != AVERROR(EBADF)) {
+fprintf(stderr,
+"avio_check on invalid FD expected to return %i "
+"but %i was received\n",
+AVERROR(EBADF),
+check_invalid_ret);
+ret = 1;
+}
+
+return ret;
+}
diff --git a/tests/fate/libavformat.mak b/tests/fate/libavformat.mak
index d2acb4c9e0..7a22f54c04 100644
--- a/tests/fate/libavformat.mak
+++ b/tests/fate/libavformat.mak
@@ -26,6 +26,11 @@ FATE_LIBAVFORMAT-$(CONFIG_IMF_DEMUXER) += fate-imf
fate-imf: libavformat/tests/imf$(EXESUF)
fate-imf: CMD = run libavformat/tests/imf$(EXESUF)
+FATE_LIBAVFORMAT-$(CONFIG_PIPE_PROTOCOL) += fate-pipe
[FFmpeg-devel] [PATCH v2 1/2] avformat/file: Add a specialized url_check callback for pipe protocol
Using file_check for the pipe protocol doesn't make sense. For example,
for a URL like “pipe:0” it would end up checking whether the “pipe:0”
file exists. This patch instead makes it check the access modes on the
corresponding file descriptor using fcntl on *nix and DuplicateHandle on
Windows.
v2: Use DuplicateHandle on Windows to check whether the duplicated
handle can have the corresponding access flags.
Signed-off-by: Neil Roberts
---
libavformat/file.c | 74 --
1 file changed, 71 insertions(+), 3 deletions(-)
diff --git a/libavformat/file.c b/libavformat/file.c
index 98c9e81bcb..b3f7bc9282 100644
--- a/libavformat/file.c
+++ b/libavformat/file.c
@@ -375,9 +375,8 @@ const URLProtocol ff_file_protocol = {
#if CONFIG_PIPE_PROTOCOL
-static int pipe_open(URLContext *h, const char *filename, int flags)
+static int pipe_get_fd(const char *filename, int flags)
{
-FileContext *c = h->priv_data;
int fd;
char *final;
av_strstart(filename, "pipe:", );
@@ -390,6 +389,14 @@ static int pipe_open(URLContext *h, const char *filename,
int flags)
fd = 0;
}
}
+
+return fd;
+}
+
+static int pipe_open(URLContext *h, const char *filename, int flags)
+{
+FileContext *c = h->priv_data;
+int fd = pipe_get_fd(filename, flags);
#if HAVE_SETMODE
setmode(fd, O_BINARY);
#endif
@@ -398,13 +405,74 @@ static int pipe_open(URLContext *h, const char *filename,
int flags)
return 0;
}
+static int pipe_check(URLContext *h, int mask)
+{
+int fd = pipe_get_fd(h->filename, mask);
+int access = 0;
+
+#ifdef _WIN32
+
+HANDLE pipe_handle = (HANDLE) _get_osfhandle(fd);
+HANDLE tmp_handle;
+
+if (pipe_handle == INVALID_HANDLE_VALUE)
+return AVERROR(errno);
+
+if (mask & AVIO_FLAG_READ &&
+DuplicateHandle(GetCurrentProcess(), /* hSourceProcessHandle */
+pipe_handle,
+GetCurrentProcess(), /* hTargetProcessHandle */
+_handle,
+FILE_READ_DATA,
+FALSE, /* bInheritHandle */
+0 /* options */)) {
+access |= AVIO_FLAG_READ;
+CloseHandle(tmp_handle);
+}
+
+if (mask & AVIO_FLAG_WRITE &&
+DuplicateHandle(GetCurrentProcess(), /* hSourceProcessHandle */
+pipe_handle,
+GetCurrentProcess(), /* hTargetProcessHandle */
+_handle,
+FILE_WRITE_DATA,
+FALSE, /* bInheritHandle */
+0 /* options */)) {
+access |= AVIO_FLAG_WRITE;
+CloseHandle(tmp_handle);
+}
+
+#else /* _WIN32 */
+
+int status_flags = fcntl(fd, F_GETFL);
+
+if (status_flags == -1)
+return AVERROR(errno);
+
+switch (status_flags & O_ACCMODE) {
+case O_RDONLY:
+access = AVIO_FLAG_READ;
+break;
+case O_WRONLY:
+access = AVIO_FLAG_WRITE;
+break;
+case O_RDWR:
+access = AVIO_FLAG_READ | AVIO_FLAG_WRITE;
+break;
+}
+
+#endif /* _WIN32 */
+
+return access & mask;
+}
+
const URLProtocol ff_pipe_protocol = {
.name= "pipe",
.url_open= pipe_open,
.url_read= file_read,
.url_write = file_write,
.url_get_file_handle = file_get_handle,
-.url_check = file_check,
+.url_check = pipe_check,
.priv_data_size = sizeof(FileContext),
.priv_data_class = _class,
.default_whitelist = "crypto,data"
--
2.37.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 2/2] avformat/tests: Add a test for avio_check with the pipe protocol
Creates a UNIX pipe and then verifies that avio_check returns the right
access flags for the two ends of the pipe.
Signed-off-by: Neil Roberts
---
libavformat/Makefile | 1 +
libavformat/tests/.gitignore | 1 +
libavformat/tests/pipe.c | 101 +++
tests/fate/libavformat.mak | 5 ++
4 files changed, 108 insertions(+)
create mode 100644 libavformat/tests/pipe.c
diff --git a/libavformat/Makefile b/libavformat/Makefile
index f67a99f839..9c681c58c5 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -732,6 +732,7 @@ TESTPROGS-$(CONFIG_MOV_MUXER)+= movenc
TESTPROGS-$(CONFIG_NETWORK) += noproxy
TESTPROGS-$(CONFIG_SRTP) += srtp
TESTPROGS-$(CONFIG_IMF_DEMUXER) += imf
+TESTPROGS-$(CONFIG_PIPE_PROTOCOL)+= pipe
TOOLS = aviocat \
ismindex\
diff --git a/libavformat/tests/.gitignore b/libavformat/tests/.gitignore
index cdd0cce061..567d6f9e40 100644
--- a/libavformat/tests/.gitignore
+++ b/libavformat/tests/.gitignore
@@ -7,3 +7,4 @@
/srtp
/url
/seek_utils
+/pipe
diff --git a/libavformat/tests/pipe.c b/libavformat/tests/pipe.c
new file mode 100644
index 00..68540c1a8c
--- /dev/null
+++ b/libavformat/tests/pipe.c
@@ -0,0 +1,101 @@
+/*
+ * Copyright (c) 2022 Neil Roberts
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include "libavformat/avio.h"
+#include "libavutil/error.h"
+
+static int check_pipe(const char *url, int mask, int expected)
+{
+int flags = avio_check(url, mask);
+
+if (flags < 0) {
+fprintf(stderr,
+"avio_check for %s with mask 0x%x failed: %s\n",
+url,
+mask,
+av_err2str(flags));
+return 0;
+}
+
+if (flags != expected) {
+fprintf(stderr,
+"Wrong result returned from avio_check for %s with mask 0x%x. "
+"Expected 0x%x but received 0x%x\n",
+url,
+mask,
+flags,
+expected);
+return 0;
+}
+
+return 1;
+}
+
+int main(int argc, char **argv)
+{
+int ret = 0;
+int pipe_fds[2];
+char read_url[20], write_url[20];
+int check_invalid_ret;
+
+if (pipe(pipe_fds) == -1) {
+fprintf(stderr, "error creating pipe: %s\n", strerror(errno));
+return 1;
+}
+
+snprintf(read_url, sizeof(read_url), "pipe:%d", pipe_fds[0]);
+snprintf(write_url, sizeof(write_url), "pipe:%d", pipe_fds[1]);
+
+if (!check_pipe(read_url,
+AVIO_FLAG_READ | AVIO_FLAG_WRITE,
+AVIO_FLAG_READ))
+ret = 1;
+
+if (!check_pipe(write_url,
+AVIO_FLAG_READ | AVIO_FLAG_WRITE,
+AVIO_FLAG_WRITE))
+ret = 1;
+
+/* Ensure that we don't get flags that we didn't ask for */
+if (!check_pipe(read_url, AVIO_FLAG_WRITE, 0))
+ret = 1;
+
+close(pipe_fds[0]);
+close(pipe_fds[1]);
+
+/* An invalid fd should return EBADF */
+check_invalid_ret = avio_check(read_url, AVIO_FLAG_READ);
+
+if (check_invalid_ret != AVERROR(EBADF)) {
+fprintf(stderr,
+"avio_check on invalid FD expected to return %i "
+"but %i was received\n",
+AVERROR(EBADF),
+check_invalid_ret);
+ret = 1;
+}
+
+return ret;
+}
diff --git a/tests/fate/libavformat.mak b/tests/fate/libavformat.mak
index d2acb4c9e0..7a22f54c04 100644
--- a/tests/fate/libavformat.mak
+++ b/tests/fate/libavformat.mak
@@ -26,6 +26,11 @@ FATE_LIBAVFORMAT-$(CONFIG_IMF_DEMUXER) += fate-imf
fate-imf: libavformat/tests/imf$(EXESUF)
fate-imf: CMD = run libavformat/tests/imf$(EXESUF)
+FATE_LIBAVFORMAT-$(CONFIG_PIPE_PROTOCOL) += fate-pipe
+fate-pipe: libavformat/tests/pipe$(EXESUF)
+fate-pipe: CMD = run libavformat/tests/pipe$(EXESUF)
+fate-pipe: CMP = null
+
FATE_LIBAVFORMAT += fate-seek_utils
fate-seek_utils: libavformat
[FFmpeg-devel] [PATCH 1/2] avformat/file: Add a specialized url_check callback for pipe protocol
Using file_check for the pipe protocol doesn't make sense. For example,
for a URL like “pipe:0” it would end up checking whether the “pipe:0”
file exists. This patch instead makes it check the access modes on the
corresponding file descriptor using fcntl.
Signed-off-by: Neil Roberts
---
libavformat/file.c | 40 +---
1 file changed, 37 insertions(+), 3 deletions(-)
diff --git a/libavformat/file.c b/libavformat/file.c
index 98c9e81bcb..f17d219cb2 100644
--- a/libavformat/file.c
+++ b/libavformat/file.c
@@ -375,9 +375,8 @@ const URLProtocol ff_file_protocol = {
#if CONFIG_PIPE_PROTOCOL
-static int pipe_open(URLContext *h, const char *filename, int flags)
+static int pipe_get_fd(const char *filename, int flags)
{
-FileContext *c = h->priv_data;
int fd;
char *final;
av_strstart(filename, "pipe:", );
@@ -390,6 +389,14 @@ static int pipe_open(URLContext *h, const char *filename,
int flags)
fd = 0;
}
}
+
+return fd;
+}
+
+static int pipe_open(URLContext *h, const char *filename, int flags)
+{
+FileContext *c = h->priv_data;
+int fd = pipe_get_fd(filename, flags);
#if HAVE_SETMODE
setmode(fd, O_BINARY);
#endif
@@ -398,13 +405,40 @@ static int pipe_open(URLContext *h, const char *filename,
int flags)
return 0;
}
+static int pipe_check(URLContext *h, int mask)
+{
+int fd = pipe_get_fd(h->filename, mask);
+int status_flags = fcntl(fd, F_GETFL);
+int access;
+
+if (status_flags == -1)
+return AVERROR(errno);
+
+switch (status_flags & O_ACCMODE) {
+case O_RDONLY:
+access = AVIO_FLAG_READ;
+break;
+case O_WRONLY:
+access = AVIO_FLAG_WRITE;
+break;
+case O_RDWR:
+access = AVIO_FLAG_READ | AVIO_FLAG_WRITE;
+break;
+default:
+access = 0;
+break;
+}
+
+return access & mask;
+}
+
const URLProtocol ff_pipe_protocol = {
.name= "pipe",
.url_open= pipe_open,
.url_read= file_read,
.url_write = file_write,
.url_get_file_handle = file_get_handle,
-.url_check = file_check,
+.url_check = pipe_check,
.priv_data_size = sizeof(FileContext),
.priv_data_class = _class,
.default_whitelist = "crypto,data"
--
2.37.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
