Re: [FFmpeg-devel] [PATCH 2/8] lavu: new AVWriter API
Rémi Denis-Courmont (12023-05-02): > Indirecting the printf function seems pretty pointless. The last thing you Please re-read the code, there is no other way of obtaining a va_list from an actual argument than making a call to a vararg function. > want are different implementations of printf() with different limitations. > And > it makes writing different backends needlessly complex, while you could just > use vasprintf(). > > Typically, with this kind of abstraction, only the raw bytes writing is > abstracted away. Examples include funopen() and fopencookie(). > > As for hypothetical use cases whence vasprintf() wouldb e "too slow", then > should not use printf()-style or function pointers to begin with. Besides if > you _really_ want to avoid the heap allocation, you can also use > fopencookie() > on systems that provide it or equivalent. Being portable and not doing dynamic allocation are two major points of the design, so you will excuse me if I pass on that suggestion. > That sounds like it belongs in whichever backend actually wants to heap- > allocate the output buffer, not the frontend. There is nothing about heap allocation in this code. And if code can be in the framework common to all backends, then it is where it belongs, not duplicated in each backend. > This is an abstraction inversion (and also highly inefficient) + what I noted > above. See above, it is necessary. (If you think it is not, try doing better.) > This is an analogue of puts/fputs, not "print". This is an analogue of puts, fputs and print, and I decided to call it print. > Same problems and overengineering as above. I think you are missing something important about the purpose of this code, but I cannot guess what exactly. Please be more detailed about why you think it is overengineered and point how you would do it simpler. Thanks for the comments. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 1/8] lavu: add macros to help making future-proof structures
Rémi Denis-Courmont (12023-05-02): > A JSON writer that requires forced alignment is a poorly-written JSON parser. This JSON writer requires that the structures that contain its state are aligned, just like any other piece of C code. There is nothing poorly-written here. > This is reinventing standard max_align_t but with a more confusing name and > less portable implementation... I did not know it exists. Thanks for the pointer. > Indeed this is not about "padding", but about alignment. It is about both. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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] Embedded documentation?
Hi.
Three years ago, I shared some brief thoughts about embedding the
documentation in the libraries. For example, that would allow GUI
applications to open help dialogs about specific options.
To see what it would need, I wrote the following header. I did not work
any further, because groundwork need to be laid first. But now that it
was mentioned in another thread, I think it is a good idea to show it,
to see how people like it.
Please share your remarks. Even “+1” to say you like it, because people
who will not like it will not hesitate to post “-1”.
Regards,
--
Nicolas George
typedef struct AVDocNode AVDocNode;
typedef struct AVDocLink AVDocLink;
typedef enum AVDocNodeType AVDocNodeType;
typedef enum AVDocLinkType AVDocLinkType;
/**
* Link to another documentation node.
*/
struct AVDocLink {
AVDocNode *target;
AVDocLinkType type;
};
/**
* Node in the documentation system.
*
* A node can be the description of a codec, format, filter, option, type,
* etc.
*/
struct AVDocNode {
/**
* Names of the component.
*
* It is a concatenation of 0-terminated strings, terminated by an empty
* string (i.e. a double 0).
* For example "frame_rate, rate, r" would be "frame_rate\0rate\0r\0\0".
* The first name is the main name of the component, the other are
* aliases.
* If this field is used as a plain C string, it contains only the main
* name.
*/
const char *names;
/**
* Unique identifier of the compnent.
*
* It is composed of alphanumeric characters plus underscore and slash
* and written hierarchically.
*
* For example, the width option of the scale filter would be
* "lavfi/vf_scale/opt_width".
*
* This identifier can be used for links in the text.
*
* It matches the symbol that makes the documentation available, in the
* avdoc_ namespace with double underscore standing for slashes:
* extern const AVDocNode avdoc_lavfi__vf_scale__opt_width;
*/
const char *id;
/**
* Title / short description, possibly NULL.
*
* Can be used in a table of contents for example.
*/
const char *title;
/**
* Text of the documentation in XXX Markdown / FFHTML.
*
* Apparently we want to write the documentation in Markdown or similar,
* but the build system can convert when creating the data structure to
* embed in the library.
*
* On one hand, Markdown can be dumped as is to the user, in a terminal
* or a basic dialog box.
*
* On the other hand, strict minimalist HTML is more program-friendly,
* which makes it more convenient for programs that want to display it
* with actual italics
*
* I think FFHTML (i.e. a small, strict and clearly documented subset of
* HTML) would be better.
*/
const char *text;
/**
* Object about which the documentation is.
*
* If not NULL, points to an object starting with an AVClass pointer.
*/
void *object;
/**
* Links towards other nodes.
*
* All nodes linked in the text must have an entry here, but implicit
* links are possible too, for example the type of an option.
*
* The types are ordered by type.
*/
const AVDocLink *links;
/**
* Type of the node, and of the object documented.
*/
AVDocNodeType type;
};
/**
* Type of a documentation node.
*/
enum AVDocNodeType {
AVDOC_TYPE_GENERIC = 0,
AVDOC_TYPE_MUXER,
AVDOC_TYPE_DEMUXER,
AVDOC_TYPE_ENCODER,
AVDOC_TYPE_DECODER,
AVDOC_TYPE_FILTER,
AVDOC_TYPE_BITSTREAM_FILTER,
AVDOC_TYPE_SWSCALER,
AVDOC_TYPE_SWRESAMPLER,
AVDOC_TYPE_DEVICE_VIDEO_OUTPUT,
AVDOC_TYPE_DEVICE_VIDEO_INPUT,
AVDOC_TYPE_DEVICE_AUDIO_OUTPUT,
AVDOC_TYPE_DEVICE_AUDIO_INPUT,
AVDOC_TYPE_DEVICE_OUTPUT,
AVDOC_TYPE_DEVICE_INPUT,
AVDOC_TYPE_OPTION,
AVDOC_TYPE_TYPE,
AVDOC_TYPE_SYNTAX,
AVDOC_TYPE_EXAMPLES,
AVDOC_TYPE_EXPLANATIONS,
};
/**
* Type of a link, i.e. relation between the source and the target of the
* link.
*
* More important links have a lower value.
*/
enum AVDocLinkType {
/**
* The linked node is the parent.
*
* For example, the parent the node for a private option is the node for
* the corresponding codec/format/filter.
*/
AVDOC_LINK_PARENT = 0x100,
/**
* The linked node is a subpart, section, chapter, etc.
*/
AVDOC_LINK_SUBPART = 0x200,
/**
* The linked node describes an option or an option constant.
*/
AVDOC_LINK_OPTION = 0x300,
/**
* Threshold value for the self-contained minimal documentation of an
* object.
*/
AVDOC_LINK_SELF_CONTAINED = 0x400,
/**
* The linked node is the reference for a type, syntax, etc.
*/
AVDOC_LINK_REFERENCE = 0x500,
/**
* Threshold value for the self-co
Re: [FFmpeg-devel] [PATCH 5/8] lavu: add a JSON writer API (WIP)
Leo Izen (12023-05-01): > > Yes, strings are a terrible API, but our project is not only a set of > > libraries, it is also a set of command-line tools, and command-line > > tools work with strings and nothing else. > This is a good argument for putting the code in fftools/ and not libavutil, > fwiw. This is not wrong. But I realize now my argument was widely incomplete. GUI applications not of our own still need text to communicate with users about things that do not have their specific widget, for example. The avlibraries must not only perform work for applications, they also must help applications communicate with users about that work, and that is done with text. See there for a more complete wording: http://ffmpeg.org/pipermail/ffmpeg-devel/2023-May/309077.html Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 5/8] lavu: add a JSON writer API (WIP)
Michael Niedermayer (12023-04-30): > There are many projects which use libavcodec, format, filter > Human users use these projects > > If the standarization is at a C struct level only then the human interface > for each application can be different. > Thats fine if the 2 cases use fundamentally different interfaces like a > GUI draging, droping and connecting components vs some command line > interface. > But if 2 applications both use command line / string based interfaces > it would be nice to the human user if she could use/learn the same syntax > and transfer a working example / script from one to the other. > > So i think strings do matter for C libs because of that. Thank you for stating it that way. I think I can make it even a little stronger: The API of the avlibraries is so rich that applications cannot realistically cover all of them, and this is why we have the options system: so that applications can expose all the knobs and controls of avlibs without having to maintain code for every one of them. But the options system has severe limitations, including the occasional need for half-a-dozen backslashes or more for escaping and the inability to define AV_OPT_TYPE_SOMETHING if SOMETHING is defined in another library than lavu or nor generic enough. Overcoming the limitations of the options system is a project I have had for a long time, and it connects to the project of embedding the documentation into the libraries (which has received some support). http://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/184525.html (the technical details in my mind have evolved a little, but not much) http://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/268389.html And for that, we absolutely need an efficient strings API (this is now supported by a majority of developers, thankfully) and standardized serialization functions. In the libraries, not the avtools. To say it in a more concise way: The avlibraries must not only perform work for applications, they also must help applications communicate with users about that work, and that is done with text. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 5/8] lavu: add a JSON writer API (WIP)
Anton Khirnov (12023-04-29): > libavfilter is a C library with a C API. Any structured output from > filters should be in the form of a C object, typically a struct. I do > not see why are you so in love with strings, they make for terrible > APIs. Yes, strings are a terrible API, but our project is not only a set of libraries, it is also a set of command-line tools, and command-line tools work with strings and nothing else. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 5/8] lavu: add a JSON writer API (WIP)
James Almer (12023-04-29): > It should be exporting frame metadata Not really. Exporting information to frame data is a hack, “this looks pointy, we have a hammer, let's nail it”. It only works if the data is very flat. It does not work for data that comes at the end of the stream. It still requires a way to get the data out in a parse-friendly syntax when the program running is not ffprobe. Having an API and user interface for filters to return structured data is one of the reasons I want this API in libavutil. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 5/8] lavu: add a JSON writer API (WIP)
Anton Khirnov (12023-04-29): > As far as I can see, there are exactly two places in the codebase that > produce JSON: af_loudnorm and ffprobe. I think I remember finding a few other places, but it does not matter much. Users have been asking¹ for parse-friendly output from filters for years, and JSON is often the format they prefer. Therefore, all the filters that log useful information are candidates where a JSON writing API is useful. (See my answer to James for why “put it in frame metadata” is not a real solution.) > * IMO the filter should not be doing this at all and instead produce some > sort of a struct and let the users process it as they wish And to let users process it as they wish, it needs to be formatted into a parse-friendly syntax like JSON. > ffprobe: > * is not one of the libraries, but rather their caller > * we are not in business of providing random non-multimedia-related > services to callers, unless they are useful in our libraries; > if this code is only useful in ffprobe then it should live in fftools/ This is your opinion, not the project policy. My opinion is that anything that is useful for fftools and can be made to have a properly-defined API has its place in libavutil. Let us see which opinion has more support. > * it is not at all obvious that switching ffprobe to this code would > be an improvement; a patch actually demonstrating this would be most > useful I distinctly remember writing the changes to ffprobe and getting the same output except for the indentation level, but I do not find the code. 1: To be aware what would be useful to the project, reading the users mailing-lists helps. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 08/21] fftools/ffmpeg_filter: add filtergraph private data
James Almer (12023-04-29): > History has shown that these notifications have had no effect on users, and > even on this same project's developers. A good example was ffserver.c, which > accessed an unholy amount of lavf private fields (both exposed in public > headers and even internal ones), and first_dts from AVStream, which was not > only accessed by prominent library users (One of which refuses to do things > right and forces distros to use a patch to expose said field on their ffmpeg > packages for the sake of supporting their application, thus making a pure > recompilation from our tree no longer a drop-in solution on anyone's > system), but also by ffmpeg.c, with no developer noticing it to prevent such > code being pushed. Since we are discussing types for the fftools, not librairies, what people outside the project might do is not an issue As for abuse of fields private to a part of the code by another part of the code, I think you are somewhat rewriting history here. The reason the fftools used to access private fields of the libraries is not that developers neglected the rules against using them, it is that no such rule existed when the code was written, they came later. But if you really think we need to enforce the rule, then there still are better solutions than using a separate structure and littering the code with casts. For example we can apply __attribute__((unavailable)) to the private fields except in the part of the code where they are allowed. (The attribute might not be supported on all compilers, but it is plenty enough that it is supported on the compilers used by the FATE instances that test submitted patches and the compilers used by some of us.) This is a MUCH better solution than what is proposed here. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 08/21] fftools/ffmpeg_filter: add filtergraph private data
Anton Khirnov (12023-04-29): > That does not follow at all - just because something was moved once does > not mean it cannot be moved again if a better place is found later. Good, then you should have no objection to replacing your private structure with a “/* these fields are private to… */” comment. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 5/8] lavu: add a JSON writer API (WIP)
Nicolas George (12023-04-28): > Signed-off-by: Nicolas George > --- > libavutil/Makefile | 1 + > libavutil/json.c | 368 +++ > libavutil/json.h | 470 + > 3 files changed, 839 insertions(+) > create mode 100644 libavutil/json.c > create mode 100644 libavutil/json.h I forgot to write: I wrote this code not only because we have half-baked JSON output in multiple places in the code, but also to show the kind of API AVWriter makes possible. Typical JSON APIs would have a function to write a string value or an object key, requiring the caller to build the string beforehand. Of course, this API can do that: > +void av_json_add_string(AVJson *jc, const char *str); including with a format string, which is less usual: > +void av_json_add_string_printf(AVJson *jc, const char *fmt, ...) > av_printf_format(2, 3); But these are just wrappers for convenience over the real API: > +AVWriter av_json_begin_string(AVJson *jc); It starts a string, i.e. outputs a quote, and then we get a writer to write into that string. It will be automatically escaped, no intermediate buffer will be used, and all the functions of the writer API are available, including all the av_something_write() to come to serialize our various types. Also note that this API as a whole can produce small JSON outputs without any dynamic allocation, making it suitable for once-per-frame calls, but is not limited to that. Do we *need* that: of course not, we still put “len += snprintf()” and “av_realloc()” and error checks all over the place. Now, while people maybe look at the code, there are a few things I can work on, and I wonder which one you would like to see first: - Finishing this JSON API. - A XML API: that would be useful for dashenc, movenc, ttmlenc, ffprobe and possibly others. - Add serialization functions for our various types, like I did for av_disposition_write(). - Go forward with the others API enhancements that I promised and that depend on AVWriter. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 08/21] fftools/ffmpeg_filter: add filtergraph private data
Nicolas George (12023-04-28): > And during the work of turning all into threads, opportunities to split > the structure in more logical ways with less code noise will almost > certainly present themselves. I had intended to not reply further on this, but I just had a severe case of esprit de l'ecalier, so I just add this: For example, it is entirely possible that a lot of these “private” fields could become local variables and functions parameters in the threads. That would be elegant and efficient. But now we will never know, because they will all be stuffed in a private context, according to “good practices”, and that will spare the effort of checking what their exact scope is. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 08/21] fftools/ffmpeg_filter: add filtergraph private data
James Almer (12023-04-28): > The longtime goal of this work is to have completely standalone modules > within the CLI and each of them threaded. So while ffmpeg may not be a > library, clearly defining what fields should be accessed from "the outside" > and which shouldn't would have the same effect and benefits as if it was > one, particularly ensuring no future accidental usage of fields that should > not be touched. The decoder side for example has no need to touch a frame > the filtering side is using internally, and more so if it could result in > races. > > There's also the fact Anton is maintaining this code and this helps him keep > track of the nature of each field, plus the fact this same change has been > done before elsewhere, so if you're going to argue you don't like it because > in your opinion it's unnecessary, then that's not enough grounds to block > it. (As Anton repeatedly said he does not accept the usefulness of the role of maintainer, invoking that last argument feels a little like trying to have it both ways. But since I do acknowledge the usefulness of maintainers, I do accept it.) Thanks. This is slightly more convincing than what was explained until now. But there are ways of clearly stating which fields are internal and which fields are ok for outside access without littering the code with casts, because fgp_from_fg() and co. are exactly that. And during the work of turning all into threads, opportunities to split the structure in more logical ways with less code noise will almost certainly present themselves. Anyway, I find it sad that in the recent years FFmpeg has more and more followed “good practices” not because, being good, they bring actual benefit to the code but just because they're “good practices”. We are not underskilled Java developers following the rules from the textbook without understanding their purpose; we are FFmpeg, we try new and original ways of writing code to make it more efficient and more elegant. FilterGraphPriv is neither efficient nor elegant. But if I failed to convince you or others with this mail, I will not fight this anymore. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 2/8] lavu: new AVWriter API
Rodney Baker (12023-04-28): > I'm not normally a reviewer, but I noticed a few minor grammatical things > that > stood out - hope this is OK. Thanks, it is absolutely useful. > Nit - s/compating/comparing/ Fixed. > > +**Note:** AVWriter is 8-bit clean, the strings it manipulates can be > Use a hyphen or a semicolon rather than a comma after "clean". After consideration, a semicolon would be too strong; a hyphen would be strange, too literary. > > +In mainstream C, a function that needs to return a string usually have two > > +options: either they accept pointer to a buffer that they fill or they > > +allocate the buffer themselves and return it. Both these options have > > +drawbacks, which one is best depends on the circumstances of the caller. > Semicolon instead of comma after "drawbacks". Same here, I find a semicolon would be too strong. > Drop comma after "circumstances". Done. > > +AVWriter also makes the work of the called function easier by providing > > +convenient functions to append to the string that completely wrap error > > +checks. Note that it only works for strings created as streams; functions > > +that need random access to the string already built still need to manage > > +their own buffers; some AVWriter implementations can still help for that. > Full stop after "buffers" (instead of semicolon - you've already used one > previously in the same sentence). Disagree on this one: the last part should still be in the sentence that starts with “Note that”. There is no problem in having multiple semicolons to separate parts on the same level. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] lavu: header and documentation for AVWriter
James Almer (12023-04-25): > I support the addition of a new better API as a replacement for AVBprint, as > long as you take into account the suggestions made by people and their > requests for exemplifications of its usage and usefulness. This also means > potential changes to the header. Of course. If you notice remarks that I neglected to take into consideration, please remind me of them. > And please, do not ignore or dismiss any of them. No one is out to block > your work for the sake of it. By the way, thanks for intervening on my behalf on Monday. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] lavu: header and documentation for AVWriter
Anton Khirnov (12023-04-26): > This project is developed in the open, private emails, rumors, and > hearsay do not count as arguments. > > This code is unnecessary bloat that adds nothing of value to the > project. Your negative opinion has already been taken into consideration, it is the main reason I write “significant more support than opposition” and not “no opposition at all”. -- Nicolas George signature.asc Description: PGP signature ___ 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 8/8] lavf/dump: use av_disposition_write()
It changes the output to have underscores instead of spaces.
Signed-off-by: Nicolas George
---
libavformat/dump.c | 37 +
1 file changed, 1 insertion(+), 36 deletions(-)
Note: I consider the change to backspaces good: now we can copy-paste
from dump directly into a command-line.
diff --git a/libavformat/dump.c b/libavformat/dump.c
index 96b364e337..8b55e60b6a 100644
--- a/libavformat/dump.c
+++ b/libavformat/dump.c
@@ -584,42 +584,7 @@ static void dump_stream_format(AVWriter wr,
print_fps(wr, 1 / av_q2d(st->time_base), "tbn");
}
-if (st->disposition & AV_DISPOSITION_DEFAULT)
-av_writer_printf(wr, " (default)");
-if (st->disposition & AV_DISPOSITION_DUB)
-av_writer_printf(wr, " (dub)");
-if (st->disposition & AV_DISPOSITION_ORIGINAL)
-av_writer_printf(wr, " (original)");
-if (st->disposition & AV_DISPOSITION_COMMENT)
-av_writer_printf(wr, " (comment)");
-if (st->disposition & AV_DISPOSITION_LYRICS)
-av_writer_printf(wr, " (lyrics)");
-if (st->disposition & AV_DISPOSITION_KARAOKE)
-av_writer_printf(wr, " (karaoke)");
-if (st->disposition & AV_DISPOSITION_FORCED)
-av_writer_printf(wr, " (forced)");
-if (st->disposition & AV_DISPOSITION_HEARING_IMPAIRED)
-av_writer_printf(wr, " (hearing impaired)");
-if (st->disposition & AV_DISPOSITION_VISUAL_IMPAIRED)
-av_writer_printf(wr, " (visual impaired)");
-if (st->disposition & AV_DISPOSITION_CLEAN_EFFECTS)
-av_writer_printf(wr, " (clean effects)");
-if (st->disposition & AV_DISPOSITION_ATTACHED_PIC)
-av_writer_printf(wr, " (attached pic)");
-if (st->disposition & AV_DISPOSITION_TIMED_THUMBNAILS)
-av_writer_printf(wr, " (timed thumbnails)");
-if (st->disposition & AV_DISPOSITION_CAPTIONS)
-av_writer_printf(wr, " (captions)");
-if (st->disposition & AV_DISPOSITION_DESCRIPTIONS)
-av_writer_printf(wr, " (descriptions)");
-if (st->disposition & AV_DISPOSITION_METADATA)
-av_writer_printf(wr, " (metadata)");
-if (st->disposition & AV_DISPOSITION_DEPENDENT)
-av_writer_printf(wr, " (dependent)");
-if (st->disposition & AV_DISPOSITION_STILL_IMAGE)
-av_writer_printf(wr, " (still image)");
-if (st->disposition & AV_DISPOSITION_NON_DIEGETIC)
-av_writer_printf(wr, " (non-diegetic)");
+av_disposition_write(wr, st->disposition, " (", ")", NULL, 0);
av_writer_printf(wr, "\n");
dump_metadata(wr, st->metadata, "");
--
2.39.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 7/8] lavf/options: add av_disposition_write()
TODO APIchanges entry
Signed-off-by: Nicolas George
---
libavformat/avformat.h | 14 ++
libavformat/options.c | 34 ++
2 files changed, 48 insertions(+)
This and the next patch show the kind of fruits become low-hanging
thanks to the platform of AVWriter.
Eventually, I want that every public structure and enumeration to have a
_write() function with a standardized API, and later we will be able to
use a custom %something format string in printf directly.
Another project of mine (that has AVWriter as a prerequisite) would
invert the logic of this function: instead of having the to-string and
from-string function explore the options table, we would have the
options system quety the to-string and from-string functions.
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 5302a34c0d..51fd78b71c 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -824,6 +824,20 @@ int av_disposition_from_string(const char *disp);
*/
const char *av_disposition_to_string(int disposition);
+/**
+ * Write a disposition as a string.
+ * The before and after string are written around each element of the
+ * disposition and the between string is written between them, if they are
+ * not NULL.
+ * Unknown disposition bits are written as an extra hexadecimal value.
+ * No flags are defined.
+ */
+void av_disposition_write(AVWriter wr, unsigned disposition,
+ const char *before,
+ const char *after,
+ const char *between,
+ unsigned flags);
+
/**
* Options for behavior on timestamp wrap detection.
*/
diff --git a/libavformat/options.c b/libavformat/options.c
index e4a3aceed0..a813e11060 100644
--- a/libavformat/options.c
+++ b/libavformat/options.c
@@ -29,6 +29,7 @@
#include "libavutil/internal.h"
#include "libavutil/intmath.h"
#include "libavutil/opt.h"
+#include "libavutil/writer.h"
/**
* @file
@@ -348,3 +349,36 @@ const char *av_disposition_to_string(int disposition)
return NULL;
}
+
+void av_disposition_write(AVWriter wr, unsigned disposition,
+ const char *before,
+ const char *after,
+ const char *between,
+ unsigned flags)
+{
+int sep = 0;
+
+for (const AVOption *opt = stream_options; opt->name; opt++) {
+if (option_is_disposition(opt) && (disposition &
opt->default_val.i64)) {
+if (sep && between)
+av_writer_print(wr, between);
+if (before)
+av_writer_print(wr, before);
+av_writer_print(wr, opt->name);
+if (after)
+av_writer_print(wr, after);
+disposition &= ~opt->default_val.i64;
+sep = 1;
+}
+}
+if (disposition) {
+if (sep && between)
+av_writer_print(wr, between);
+if (before)
+av_writer_print(wr, before);
+av_writer_printf(wr, "%x?", disposition);
+if (after)
+av_writer_print(wr, after);
+sep = 1;
+}
+}
--
2.39.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 6/8] lavu: add JSON writer test (WIP)
Signed-off-by: Nicolas George
---
libavutil/Makefile | 1 +
libavutil/tests/json.c | 139 +++
tests/fate/libavutil.mak | 4 ++
3 files changed, 144 insertions(+)
create mode 100644 libavutil/tests/json.c
diff --git a/libavutil/Makefile b/libavutil/Makefile
index a8a9700778..8800d25831 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -247,6 +247,7 @@ TESTPROGS = adler32
\
hwdevice\
integer \
imgutils\
+json\
lfg \
lls \
log \
diff --git a/libavutil/tests/json.c b/libavutil/tests/json.c
new file mode 100644
index 00..4e4106889e
--- /dev/null
+++ b/libavutil/tests/json.c
@@ -0,0 +1,139 @@
+/*
+ * Copyright (c) 2021 Nicolas George
+ *
+ * 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 "libavutil/json.h"
+#include "libavutil/opt.h"
+
+static void print_success(AVWriter wr)
+{
+printf(" (%s)\n", av_writer_get_error(wr, 0) ? "error" : "ok");
+}
+
+static void test_escape_writer(void)
+{
+AVWriter out = av_stdio_writer(stdout);
+AVWriter wr;
+
+wr = av_json_escape_writer(out, 0);
+av_writer_printf(wr, "Test of the \"JSON\" writer.\n"
+ "It will \aring.\n"
+ "c∈UBMP.\n"
+ "턞 in UTF-16");
+print_success(wr);
+wr = av_json_escape_writer(out, AV_JSON_FLAG_BAD_ENCODING_REPLACE);
+av_writer_printf(wr, "Text\bbadly\222encoded");
+print_success(wr);
+}
+
+static void test_json_context(void)
+{
+AVJson *jc = AV_JSON_DEFINE(8);
+AVWriter out = av_stdio_writer(stdout);
+
+av_opt_set_int(jc, "indent", 2, 0);
+av_json_init(jc, out, 0, NULL);
+av_json_begin_object(jc);
+ av_json_add_string(jc, "filename");
+ av_json_add_string(jc, "example.nut");
+ av_json_add_string(jc, "streams");
+ av_json_begin_array(jc);
+av_json_begin_object(jc);
+ av_json_add_string(jc, "type");
+ av_json_add_string(jc, "video");
+av_json_end_object(jc);
+av_json_add_string(jc, "null");
+av_json_begin_object(jc);
+ av_json_add_string(jc, "type");
+ av_json_add_string(jc, "audio");
+av_json_end_object(jc);
+ av_json_end_array(jc);
+ av_json_add_string(jc, "duration");
+ av_json_add_string(jc, "42");
+av_json_end_object(jc);
+}
+
+static unsigned rnd_next(unsigned *rnd, unsigned bits)
+{
+*rnd = (*rnd * 1664525 + 1013904223) & 0x;
+return *rnd >> (32 - bits);
+}
+
+static void test_json_context_rec(AVJson *jc, unsigned max_depth, unsigned
obj, unsigned *rnd)
+{
+unsigned n, i, t, d;
+
+n = rnd_next(rnd, 3);
+for (i = 0; i < n; i++) {
+if (obj)
+av_json_add_string_printf(jc, "key %d/%d[%d]", i, n, max_depth);
+t = rnd_next(rnd, max_depth > 0 ? 3 : 2);
+switch (t) {
+case 0:
+av_json_add_string_printf(jc, "random string #%u", rnd_next(rnd,
32));
+break;
+case 1:
+av_json_add_int(jc, rnd_next(rnd, 32));
+break;
+case 2:
+d = rnd_next(rnd, 4) + 8;
+av_json_add_double(jc, rnd_next(rnd, 32) / (double)(1 << d));
+break;
+case 3:
+av_json_add_bool(jc, rnd_next(rnd, 1));
+break;
+case 4:
+case 5:
+av_json_begin_object(jc);
+test_json_context_rec(jc, max_depth - 1, 1, rnd);
+av_json_
[FFmpeg-devel] [PATCH 5/8] lavu: add a JSON writer API (WIP)
Signed-off-by: Nicolas George
---
libavutil/Makefile | 1 +
libavutil/json.c | 368 +++
libavutil/json.h | 470 +
3 files changed, 839 insertions(+)
create mode 100644 libavutil/json.c
create mode 100644 libavutil/json.h
diff --git a/libavutil/Makefile b/libavutil/Makefile
index 4526ec80ca..a8a9700778 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -140,6 +140,7 @@ OBJS = adler32.o
\
imgutils.o \
integer.o\
intmath.o\
+ json.o \
lfg.o\
lls.o\
log.o\
diff --git a/libavutil/json.c b/libavutil/json.c
new file mode 100644
index 00..7ee68aaa4a
--- /dev/null
+++ b/libavutil/json.c
@@ -0,0 +1,368 @@
+/*
+ * Copyright (c) 2021 Nicolas George
+ *
+ * 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 "avassert.h"
+#include "common.h"
+#include "json.h"
+#include "opt.h"
+
+#define FIELDOK(st, f) ((char *)(&(st)->f + 1) <= (char *)(st) +
(st)->self_size)
+
+#define json_assert_abi(jwr) av_assert1(FIELDOK(jwr, padding))
+#define json_escape_writer_assert_abi(jwr) av_assert1(FIELDOK(jwr, padding))
+
+AVClass *av_json_get_class(void)
+{
+return NULL;
+}
+
+int av_json_alloc(AVJson **jc, unsigned max_depth)
+{
+return AVERROR_BUG;
+}
+
+void av_json_free(AVJson **jc)
+{
+}
+
+AVJson *av_json_preinit(AVJson *jc, AVJsonEscapeWriter *jwr)
+{
+json_assert_abi(jc);
+json_escape_writer_assert_abi(jwr);
+jc->av_class = av_json_get_class();
+jc->escape_writer = jwr;
+av_opt_set_defaults(jc);
+return jc;
+}
+
+void av_json_init(AVJson *jc, AVWriter wr, unsigned flags, AVDictionary
*options)
+{
+jc->out = wr;
+jc->flags = flags;
+jc->in_string = 0;
+jc->in_object = 0;
+jc->first_element = 1;
+jc->depth = 0;
+jc->stack = NULL;
+}
+
+static inline int check_value_must_be_string(AVJson *jc, int string)
+{
+return !jc->object_key || string;
+}
+
+static inline int check_stack_clean(AVJsonStack *stack)
+{
+return !stack->prev;
+}
+
+static inline int check_begin_end_balanced(AVJson *jc, int obj)
+{
+return jc->in_object == obj && jc->stack;
+}
+
+static inline int flag_pretty_print(AVJson *jc)
+{
+return !!(jc->flags & AV_JSON_FLAG_PRETTY_PRINT);
+}
+
+static void end_value(AVJson *jc)
+{
+if (!jc->stack && flag_pretty_print(jc))
+av_writer_print(jc->out, "\n");
+}
+
+static void auto_end_string(AVJson *jc)
+{
+if (jc->in_string) {
+av_writer_print(jc->out, "\"");
+jc->in_string = 0;
+end_value(jc);
+}
+}
+
+static void auto_add_separator(AVJson *jc)
+{
+int indent = flag_pretty_print(jc);
+
+auto_end_string(jc);
+if (!jc->first_element) {
+if (jc->object_key) {
+av_writer_print(jc->out, flag_pretty_print(jc) ? " : " : ":");
+indent = jc->object_key = 0;
+} else {
+av_writer_print(jc->out, ",");
+jc->object_key = jc->in_object;
+}
+}
+if (indent) {
+if (jc->stack)
+av_writer_print(jc->out, "\n");
+av_writer_add_chars(jc->out, ' ', 3 * jc->depth);
+}
+jc->first_element = 0;
+}
+
+static void begin_value(AVJson *jc, int string)
+{
+auto_add_separator(jc);
+av_assert1(check_value_must_be_string(jc, string));
+}
+
+static void begin_compound(AVJson *jc, AVJsonStack *stack, unsigned obj)
+{
+av_assert1(FIELDOK(stack, in_object));
+av_assert1(check_stack_clean(stack));
+stack->prev = jc->stack;
+stack
[FFmpeg-devel] [PATCH 4/8] lavf/dump: use a writer
Signed-off-by: Nicolas George
---
libavformat/avformat.h | 21 +++
libavformat/dump.c | 318 +
2 files changed, 185 insertions(+), 154 deletions(-)
This is meant to be an example, I chose a piece of code that does a lot
of strings but does not change much (to avoid rebase conflicts).
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 1916aa2dc5..5302a34c0d 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -317,6 +317,7 @@
#include "libavutil/dict.h"
#include "libavutil/log.h"
+#include "libavutil/writer.h"
#include "avio.h"
#include "libavformat/version_major.h"
@@ -2638,6 +2639,26 @@ void av_dump_format(AVFormatContext *ic,
const char *url,
int is_output);
+/**
+ * Write detailed information about the input or output format, such as
+ * duration, bitrate, streams, container, programs, metadata, side data,
+ * codec and time base, to an AVWriter.
+ *
+ * @param wr the writer where the format should be written
+ * @param ic the context to analyze
+ * @param index index of the stream to dump information about
+ * @param urlthe URL to print, such as source or destination file
+ * @param flags formatting flags, see below
+ */
+void av_dump_format_to_writer(struct AVWriter wr,
+ AVFormatContext *ic, int index,
+ const char *url, unsigned flags);
+
+/**
+ * Flag to av_dump_format_to_writer():
+ * if set, the cotext is output; if unset, the context is input.
+ */
+#define AV_DUMP_FORMAT_IS_OUTPUT 1
#define AV_FRAME_FILENAME_FLAGS_MULTIPLE 1 ///< Allow multiple %d
diff --git a/libavformat/dump.c b/libavformat/dump.c
index d31e4c2ec6..96b364e337 100644
--- a/libavformat/dump.c
+++ b/libavformat/dump.c
@@ -34,6 +34,7 @@
#include "libavutil/spherical.h"
#include "libavutil/stereo3d.h"
#include "libavutil/timecode.h"
+#include "libavutil/writer.h"
#include "libavcodec/avcodec.h"
@@ -121,45 +122,45 @@ void av_pkt_dump_log2(void *avcl, int level, const
AVPacket *pkt, int dump_paylo
}
-static void print_fps(double d, const char *postfix)
+static void print_fps(AVWriter wr, double d, const char *postfix)
{
uint64_t v = lrintf(d * 100);
if (!v)
-av_log(NULL, AV_LOG_INFO, "%1.4f %s", d, postfix);
+av_writer_printf(wr, "%1.4f %s", d, postfix);
else if (v % 100)
-av_log(NULL, AV_LOG_INFO, "%3.2f %s", d, postfix);
+av_writer_printf(wr, "%3.2f %s", d, postfix);
else if (v % (100 * 1000))
-av_log(NULL, AV_LOG_INFO, "%1.0f %s", d, postfix);
+av_writer_printf(wr, "%1.0f %s", d, postfix);
else
-av_log(NULL, AV_LOG_INFO, "%1.0fk %s", d / 1000, postfix);
+av_writer_printf(wr, "%1.0fk %s", d / 1000, postfix);
}
-static void dump_metadata(void *ctx, const AVDictionary *m, const char *indent)
+static void dump_metadata(AVWriter wr, const AVDictionary *m, const char
*indent)
{
if (m && !(av_dict_count(m) == 1 && av_dict_get(m, "language", NULL, 0))) {
const AVDictionaryEntry *tag = NULL;
-av_log(ctx, AV_LOG_INFO, "%sMetadata:\n", indent);
+av_writer_printf(wr, "%sMetadata:\n", indent);
while ((tag = av_dict_iterate(m, tag)))
if (strcmp("language", tag->key)) {
const char *p = tag->value;
-av_log(ctx, AV_LOG_INFO,
+av_writer_printf(wr,
"%s %-16s: ", indent, tag->key);
while (*p) {
size_t len = strcspn(p, "\x8\xa\xb\xc\xd");
-av_log(ctx, AV_LOG_INFO, "%.*s", (int)(FFMIN(255, len)),
p);
+av_writer_printf(wr, "%.*s", (int)(FFMIN(255, len)), p);
p += len;
-if (*p == 0xd) av_log(ctx, AV_LOG_INFO, " ");
-if (*p == 0xa) av_log(ctx, AV_LOG_INFO, "\n%s %-16s: ",
indent, "");
+if (*p == 0xd) av_writer_printf(wr, " ");
+if (*p == 0xa) av_writer_printf(wr, "\n%s %-16s: ",
indent, "");
if (*p) p++;
}
-av_log(ctx, AV_LOG_INFO, "\n");
+av_writer_printf(wr, "\n");
}
}
}
/* param change side data*/
-static void dump_paramchange(void *ctx, const AVPacketSideData *sd)
+static void dump_paramchange(AVWriter wr, const AVPacketSideData *sd)
{
int size = sd->size;
const uint8_t *data = sd->data;
@@ -184,7 +185,7 @@ FF_DISABLE_DEPRECATION_WARNINGS
channels
[FFmpeg-devel] [PATCH 3/8] lavu/writer: add test
Signed-off-by: Nicolas George
---
libavutil/Makefile | 1 +
libavutil/tests/.gitignore | 1 +
libavutil/tests/writer.c | 192 +
tests/fate/libavutil.mak | 4 +
tests/ref/fate/writer | 36 +++
5 files changed, 234 insertions(+)
create mode 100644 libavutil/tests/writer.c
create mode 100644 tests/ref/fate/writer
diff --git a/libavutil/Makefile b/libavutil/Makefile
index eb8de1dfbc..4526ec80ca 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -269,6 +269,7 @@ TESTPROGS = adler32
\
uuid\
xtea\
tea \
+writer \
TESTPROGS-$(HAVE_THREADS)+= cpu_init
TESTPROGS-$(HAVE_LZO1X_999_COMPRESS) += lzo
diff --git a/libavutil/tests/.gitignore b/libavutil/tests/.gitignore
index 87895912f5..7559e7a24c 100644
--- a/libavutil/tests/.gitignore
+++ b/libavutil/tests/.gitignore
@@ -51,3 +51,4 @@
/utf8
/uuid
/xtea
+/writer
diff --git a/libavutil/tests/writer.c b/libavutil/tests/writer.c
new file mode 100644
index 00..fdb8c4d703
--- /dev/null
+++ b/libavutil/tests/writer.c
@@ -0,0 +1,192 @@
+/*
+ * Copyright (c) 2019 Nicolas George
+ *
+ * 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 "libavutil/avassert.h"
+#include "libavutil/error.h"
+#include "libavutil/log.h"
+#include "libavutil/writer.h"
+
+/* see 69bd73b3ff873abb43de9db062b04425de153643 */
+
+/* AVWriter with only get_buffer/advance_buffer */
+
+static char *getbuf_get_buffer(AVWriter wr, size_t min_size, size_t *size);
+static void getbuf_advance_buffer(AVWriter wr, size_t size);
+
+AV_WRITER_DEFINE_METHODS(static, Getbuf, getbuf) {
+.self_size= sizeof(AVWriterMethods),
+.name = "Getbuf",
+.get_buffer = getbuf_get_buffer,
+.advance_buffer = getbuf_advance_buffer,
+};
+
+static char *getbuf_get_buffer(AVWriter wr, size_t min_size, size_t *size)
+{
+AVBufWriter *bwr = wr.obj;
+
+av_assert0(getbuf_check(wr));
+*size = bwr->size - 1 - bwr->pos;
+return bwr->buf + bwr->pos;
+}
+
+static void getbuf_advance_buffer(AVWriter wr, size_t size)
+{
+AVBufWriter *bwr = wr.obj;
+
+bwr->pos += size;
+bwr->buf[bwr->pos] = 0;
+}
+
+/* AVWriter with only write */
+
+static void write_write(AVWriter wr, const char *data, size_t size);
+
+AV_WRITER_DEFINE_METHODS(static, Write, write) {
+.self_size= sizeof(AVWriterMethods),
+.name = "Write",
+.write= write_write,
+};
+
+static void write_write(AVWriter wr, const char *data, size_t size)
+{
+AVBufWriter *bwr = wr.obj;
+
+av_assert0(write_check(wr));
+size = FFMIN(bwr->size - 1 - bwr->pos, size);
+memcpy(bwr->buf + bwr->pos, data, size);
+bwr->pos += size;
+bwr->buf[bwr->pos] = 0;
+}
+
+/* AVWriter with only vprintf */
+
+static void vprintf_vprintf(AVWriter wr, const char *fmt, va_list va);
+
+AV_WRITER_DEFINE_METHODS(static, Vprintf, vprintf) {
+.self_size= sizeof(AVWriterMethods),
+.name = "Vprintf",
+.vprintf = vprintf_vprintf,
+};
+
+static void vprintf_vprintf(AVWriter wr, const char *fmt, va_list va)
+{
+AVBufWriter *bwr = wr.obj;
+int ret;
+
+av_assert0(vprintf_check(wr));
+ret = vsnprintf(bwr->buf + bwr->pos, bwr->size - bwr->pos, fmt, va);
+if (ret > 0)
+bwr->pos += ret;
+}
+
+/* Tests */
+
+static void stress_writer(AVWriter wr)
+{
+av_writer_add_chars(wr, '*', 72);
+av_writer_add_chars(wr, '\n', 1);
+av_writer_print(wr, "Stressing the writer\n");
+av_writer_printf(wr, "Answer: %d\n", 42);
+av_writer_printf(wr, "Question: %0600d * %0600d\n", 6, 9); /* >
sizeof(AVBPrint) */
+av_writer_add_chars(wr, '*', 72);
+av_writer_add_chars(wr, '\n', 1);
[FFmpeg-devel] [PATCH 2/8] lavu: new AVWriter API
Signed-off-by: Nicolas George
---
doc/avwriter_intro.md | 186
libavutil/Makefile| 2 +-
libavutil/writer.c| 458 +++
libavutil/writer.h| 488 ++
4 files changed, 1133 insertions(+), 1 deletion(-)
create mode 100644 doc/avwriter_intro.md
create mode 100644 libavutil/writer.c
create mode 100644 libavutil/writer.h
diff --git a/doc/avwriter_intro.md b/doc/avwriter_intro.md
new file mode 100644
index 00..0e092246a2
--- /dev/null
+++ b/doc/avwriter_intro.md
@@ -0,0 +1,186 @@
+# Quick start guide for AVWriter
+
+AVWriter is an API to unify functions returning strings and to make building
+strings from parts easier. In this document, you will find an introduction
+on how to *use* AVWriter, mostly in the form of code snippets compating
+mainstream C solutions with their AVWriter counterpart.
+
+**Note:** AVWriter is 8-bit clean, the strings it manipulates can be buffers
+of binary data. The documentation is mostly written uing the vocabulary of
+strings for simplicity.
+
+In mainstream C, a function that needs to return a string usually have two
+options: either they accept pointer to a buffer that they fill or they
+allocate the buffer themselves and return it. Both these options have
+drawbacks, which one is best depends on the circumstances of the caller.
+
+AVWriter lets the caller choose the option best suited to the circumstances,
+among a small variety of built-in options or custom implementations,
+including on-the-fly compression or escaping and direct writing to a file.
+The first built-in implementation, where the strings is stored in a
+dynamically-allocated buffer, includes the optimization that small strings
+are kept on the stack.
+
+AVWriter also makes the work of the called function easier by providing
+convenient functions to append to the string that completely wrap error
+checks. Note that it only works for strings created as streams; functions
+that need random access to the string already built still need to manage
+their own buffers; some AVWriter implementations can still help for that.
+
+## I want a `char*` buffer, the function wants an AVWriter
+
+Old-style code:
+
+```
+char *buf;
+ret = av_something_to_string(, something);
+if (ret < 0)
+die("Failed");
+use_string(buf);
+av_freep();
+```
+
+Equivalent code with AVWriter:
+
+```
+AVWriter wr = av_dynbuf_writer();
+av_something_write(wr, something, 0);
+if (av_writer_get_error(wr, 0) < 0)
+die("Failed");
+use_string(av_dynbuf_writer_get_data(wr, NULL));
+av_dynbuf_writer_finalize(wr, NULL, NULL);
+```
+
+If the string is small enough, no dynamic memory allocation happens.
+
+The NULL to `av_dynbuf_writer_get_data()` can be used to retrieve the size
+of the data in the buffer.
+
+Calling `av_writer_get_error()` is mandatory.
+
+## I want a *persistent* `char*` buffer, the function wants an AVWriter
+
+Old-style code:
+
+```
+char *buf;
+ret = av_something_to_string(, something);
+if (ret < 0)
+die("Failed");
+ctx->string = buf;
+```
+
+Equivalent code with AVWriter:
+
+```
+AVWriter wr = av_dynbuf_writer();
+av_something_write(wr, something, 0);
+ret = av_dynbuf_writer_finalize(wr, >string, NULL);
+if (ret < 0)
+die("Failed");
+```
+
+## I have a `char[]` buffer, the function wants an AVWriter
+
+Old-style code:
+
+```
+char buf[BUF_SIZE];
+av_something_to_string(buf, sizeof(buf), something);
+use_string(buf);
+```
+
+Equivalent code with AVWriter:
+
+```
+char buf[BUF_SIZE];
+av_something_write(av_buf_writer(buf, sizeof(buf)), something, 0);
+use_string(buf);
+```
+
+## I need to build a string from parts
+
+Old-style code:
+
+```
+char buf[1024];
+int pos = 0;
+pos += snprintf(buf + pos, sizeof(buf) - pos,
+"Stream %d: ", i);
+av_get_channel_layout_string(buf + pos, sizeof(buf) - pos,
+ nb, layout);
+pos += strlen(buf + pos);
+pos += snprintf(buf + pos, sizeof(buf) - pos,
+", %s", av_get_sample_fmt_name(fmt));
+```
+
+Note: this code could overflow the buffer.
+
+Equivalent code with AVWriter:
+
+```
+AVWriter wr = av_dynbuf_writer();
+av_writer_printf(wr, "Stream %d: ", i);
+av_channel_layout_write(wr, nb, layout, 0);
+av_writer_printf(wr, ", %s", av_get_sample_fmt_name(fmt));
+```
+
+See the first example on how to access the resulting string.
+
+Note: this is very similar to using AVBPrint; from this side, AVWriter
+replaces AVBPrint.
+
+## I am writing the function that returns a string
+
+Old-sty
[FFmpeg-devel] [PATCH 1/8] lavu: add macros to help making future-proof structures
Signed-off-by: Nicolas George
---
libavutil/extendable.h | 59 ++
1 file changed, 59 insertions(+)
create mode 100644 libavutil/extendable.h
FFReservedPadding is used by the WIP JSON writer.
diff --git a/libavutil/extendable.h b/libavutil/extendable.h
new file mode 100644
index 00..79980fa202
--- /dev/null
+++ b/libavutil/extendable.h
@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2021 The FFmpeg project
+ *
+ * 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
+ */
+
+#ifndef AVUTIL_EXTENDABLE_H
+#define AVUTIL_EXTENDABLE_H
+
+/**
+ * @defgroup ff_extendable FFExtendable
+ *
+ * Types and macros to help designing structures that can be allocated by
+ * the application, including on the stack, but will not break ABI when
+ * extendded.
+ *
+ * This should not be used outside FFmpeg.
+ *
+ * @{
+ */
+
+/**
+ * Define a value of type as a compound literal (hidden local variable)
+ * with the field self_size filled.
+ */
+#define FF_NEW_SZ(type) ((type){ .self_size = sizeof(type) })
+
+/**
+ * Type suitable for paddign at the end of a structure, with maximum
+ * alignment.
+ */
+typedef union FFReservedPadding {
+union {
+double d;
+void *p;
+void (*f)(void);
+intmax_t i;
+} dummy;
+} FFReservedPadding;
+
+/**
+ * @}
+ */
+
+#endif /* AVUTIL_EXTENDABLE_H */
--
2.39.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".
Re: [FFmpeg-devel] [PATCH 12/21] fftools/ffmpeg_filter: keep track of filtergraph input timebase
Anton Khirnov (12023-04-27):
> Will be useful in following commits.
> ---
> fftools/ffmpeg_filter.c | 16
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
> index 6b92bc074e..3c6c580093 100644
> --- a/fftools/ffmpeg_filter.c
> +++ b/fftools/ffmpeg_filter.c
> @@ -55,6 +55,8 @@ static FilterGraphPriv *fgp_from_fg(FilterGraph *fg)
> typedef struct InputFilterPriv {
> InputFilter ifilter;
>
> +AVRational time_base;
> +
> AVFifo *frame_queue;
> } InputFilterPriv;
Just put it in InputFilter.
--
Nicolas George
signature.asc
Description: PGP signature
___
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".
Re: [FFmpeg-devel] [PATCH 11/21] fftools/ffmpeg_filter: add InputFilter private data
Anton Khirnov (12023-04-27): > Move InputFilter.frame_queue to it, which is not accessed outside of > ffmpeg_filter. > --- > fftools/ffmpeg.h| 2 -- > fftools/ffmpeg_filter.c | 33 - > 2 files changed, 24 insertions(+), 11 deletions(-) Same as FilterGraph: compleely useless change. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 09/21] fftools/ffmpeg_filter: make graph_desc private
Anton Khirnov (12023-04-27): > It is not used outside of ffmpeg_filter. > --- > fftools/ffmpeg.h| 1 - > fftools/ffmpeg_filter.c | 22 ++ > 2 files changed, 14 insertions(+), 9 deletions(-) Completely useless change. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 08/21] fftools/ffmpeg_filter: add filtergraph private data
Anton Khirnov (12023-04-27): > Start by moving OutputStream.filtered_frame to it, which really belongs > to the filtergraph rather than the output stream. Then just move them to OutputStream. This is just code noise for no practical benefit, since ffmpeg is not a library and does not have to deal with ABI stability or types declared in private headers. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 2/2] lavu/avassert: include config.h
Hendrik Leppkes (12023-04-26): > This is an installed header, it cannot depend on config.h Thanks for spotting it, that was counter-intuitive since config.h is almost indispensable to its workings. Updated version attached, similar to what exists in common.h. Regards, -- Nicolas George From e1a5ac8ba11d9321e3d0f8bebded805c779a17c1 Mon Sep 17 00:00:00 2001 From: Nicolas George Date: Wed, 26 Apr 2023 14:29:29 +0200 Subject: [PATCH] lavu/avassert: include config.h Fix setting the assert level. Signed-off-by: Nicolas George --- libavutil/avassert.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libavutil/avassert.h b/libavutil/avassert.h index 51e462bbae..1895fb7551 100644 --- a/libavutil/avassert.h +++ b/libavutil/avassert.h @@ -28,6 +28,9 @@ #define AVUTIL_AVASSERT_H #include +#ifdef HAVE_AV_CONFIG_H +# include "config.h" +#endif #include "log.h" #include "macros.h" -- 2.39.2 signature.asc Description: PGP signature ___ 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] lavu/avassert: include config.h
Fix setting the assert level. Signed-off-by: Nicolas George --- libavutil/avassert.h | 1 + 1 file changed, 1 insertion(+) diff --git a/libavutil/avassert.h b/libavutil/avassert.h index 51e462bbae..8f3f72c80c 100644 --- a/libavutil/avassert.h +++ b/libavutil/avassert.h @@ -28,6 +28,7 @@ #define AVUTIL_AVASSERT_H #include +#include "config.h" #include "log.h" #include "macros.h" -- 2.39.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 1/2] lavc/intrax8: fix an assert
Signed-off-by: Nicolas George --- libavcodec/intrax8.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/intrax8.c b/libavcodec/intrax8.c index e4c8b96c9c..c5c6727282 100644 --- a/libavcodec/intrax8.c +++ b/libavcodec/intrax8.c @@ -109,7 +109,7 @@ static inline void x8_select_ac_table(IntraX8Context *const w, int mode) table_index = get_bits(w->gb, 3); // 2 modes use same tables w->j_ac_vlc_table[mode] = j_ac_vlc[w->quant < 13][mode >> 1][table_index].table; -av_assert2(w->j_ac_vlc[mode]); +av_assert2(j_ac_vlc[mode]); } static inline int x8_get_orient_vlc(IntraX8Context *w) -- 2.39.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".
Re: [FFmpeg-devel] [PATCH] lavu: header and documentation for AVWriter
Hi. I finally have some tome to go at this again. Totalizing this thread, the previous discussions and the few private mails I received, I conclude there is significant more support than opposition to AVWriter, so I am moving forward with polishing and pushing the implementation. (To avoid antagonizing people, and because I knew a new job would mean little time to work on it soon, I refrained from pushing the header, but consider it done anyway: the fact that the implementation of AVWriter is not present in FFmpeg is now a bug that needs to be fixed.) Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH v2] fftools/ffmpeg_mux: fix reporting muxer EOF as error
Anton Khirnov (12023-04-24): > I think the point on which we disagree is your notion of "error > conditions" as being basically interchangeable. The way everything, FFmpeg and all other sane system work, is that the caller handles the very few errors it know how to handle (EAGAIN certainly, maybe a few other depending on the situation), and everything else is passed to the user in human-readable form, in the hope the user can deal with it. So in a sence, most errors are basically interchangeable for the application, and it is how it is supposed to be. For example EISDIR, ENOENT, ENOTDIR are almost always interchangeable for the application, and really mean to the user “you mistyped the file name somehow”. > In both of these cases the CLI code as it is now is correct. Absolutely not. The documentation states that muxers return errors and nothing else, there is no special case for EOF, unlike demuxers, and therefore this patch is a waste of code. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 23/25] fftools/ffmpeg_filter: add filtergraph private data
Anton Khirnov (12023-04-24): > No actual explanation then. So I suppose what you mean is that different > rules apply to you and everyone else. No. I have said what I mean, I will not let you distort it to suit your needs. > I will, the overhead code required is trivial compared to the code being > wrapped. So there is overhead code. Thank you. > > Will you discuss that having to remember which structure a field belongs > > to is an extra effort? > I will, not having to consider whether a field is accessed from other parts > of the code significantly reduces the mental load required. That is the > whole point of information hiding. I see you have not actually refuted my point, so I consider it acted too. So, we have established this patch incurs a cost. Now you need to prove it brings a benefit. Not a hypothetical benefit like “whole point of information hiding”, but an actual immediate benefit. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH v2] fftools/ffmpeg_mux: fix reporting muxer EOF as error
Marton Balint (12023-04-24): > The API change is that muxers are no longer allowed to return AVERROR_EOF > for an error condition. There is no API change because applications are not allowed to write muxers. At worst, it would be an internal API change. But it is not even an internal API change, because AVERROR_EOF was not allowed as a return code from a muxer: muxers return error codes, and AVERROR_EOF, despite being negative, is not an error. But in the end, you are right that the reasons given here make this patch wrong. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 23/25] fftools/ffmpeg_filter: add filtergraph private data
Anton Khirnov (12023-04-24): > I have no idea what you mean by this. Try a little harder. > You keep using these words. I don't think they mean what you think they > mean. Yeah, Princess Bride references are a real boost to weak arguments. > The only fact here is that the quoted paragraph is YOUR PERSONAL > OPINION, which ZERO other developers expressed support for. Or maybe I am the only one not fed up of arguing with walls. Will you dispute that each split of a structure is extra code in many places? Will you discuss that having to remember which structure a field belongs to is an extra effort? > On the other hand, the approach under discussion has explicit support > from multiple highly active developers. I have not seen this specific aspect of the issue discussed. > You are too late, many such changes have already been pushed in the last > several years. Nobody except you opposes them and the likelihood of > reversing them is extremely low. This whole thread is a pointless waste > of time that could be spend doing something actually useful. It is never too late to stop making things worse. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 23/25] fftools/ffmpeg_filter: add filtergraph private data
Anton Khirnov (12023-04-24): > So when I wanted to make changes to libavfilter recently, you claimed > your familiarity with the code makes you more qualified to judge > readability. Now my familiarity with the code makes me LESS qualified. > Curious. There is a difference between long-term knowledge of a large part of the code and a short-term acquaintance with a limited slice of the code, I hope you realize. > We've also been moving private state to private data for many years now > and none of your conjectured concerns materialized, to the contrary code > became easier to maintain. Untrue. For example, every instance of FFFormatContext in the code gives places where the code has become that much more annoying to maintain. Maybe the same code has become more maintainable at the same time due to other changes, but the fact remains that these changes make it harder to work on the code. > Now that would be pure noise. The only noise here is all the fgp_from_fg() you want to liter over the code and the extra variables it requires. > I have no idea what are you even objecting to. What is even > controversial about not exposing state that does not need to be exposed? I have explained time and again: I oppose to any change that requires us to remember or check which structure a given field belongs to when it is not already obvious by its semantic. And again, there is nothing exposed to hide here. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 23/25] fftools/ffmpeg_filter: add filtergraph private data
Anton Khirnov (12023-04-24): > What exactly is less readable? One variable gets its scope reduced, that > is a win in my book. Having to remember if a field is in structure x or structure y s a net loss that you do not see only because you just wrote the code and have everything freshly in mind. > And obviously I'm not stopping there, other things > will be moved to this private struct in future patches, Then move them into FilterGraph itself. If at some point later it makes sense to separate this structure to make the code clearer, we can do it at that time. If it happens, I predict the split will not be along the same “public”/“private” distinction you are trying to make right now. > I just prefer to > avoid giant patchsets when possible. Yes, please do, giant patchsets are rude. > Also note that exactly the same pattern is used in ffmpeg_demux, > ffmpeg_mux, and ffmpeg_enc (and soon in ffmpeg_dec). If I had noticed it at the time, I would have objected the same way. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 23/25] fftools/ffmpeg_filter: add filtergraph private data
Anton Khirnov (12023-04-24): > We'll have to disagree about this then. I belive lack of proper > structure is currently the biggest problem in ffmpeg CLI. I do not disagree on this. But this patch does not help, it just makes the code locally less readable. If it is to be useful at all, it is at least severely premature. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 23/25] fftools/ffmpeg_filter: add filtergraph private data
Anton Khirnov (12023-04-24): > Separating internal state from public interfaces is a fundamental notion > of object-oriented programming and is completely orthogonal to that > object being a part of a library or not. Using object-oriented programming to its fullest when it is not needed is a fundamental notion of mediocre programming. This patch only adds noise. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 04/25] fftools/ffmpeg_enc: always use video frame durations when available
Anton Khirnov (12023-04-19): > Previously they would only be used with trivial filtergraphs, because > filters did not handle frame durations. That is no longer true - most > filters process frame durations properly (there may still be some that > don't - this change will help finding and fixing them). > > Improves output video frame durations in a number of FATE tests. I do not object to this patch nor the one to concat, but I want to state once again: The authoritative source of information about frame durations in libavfilter, be it internally for the caller, is the timestamp of the next frame, or the timestamp of EOF for the last frame. Filtered frame durations are only a hint. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 23/25] fftools/ffmpeg_filter: add filtergraph private data
Anton Khirnov (12023-04-19): > Start by moving OutputStream.filtered_frame to it, which really belong > to the filtergraph rather than the output stream. What is the point of this? fftools/ffmpeg_filter: is not part of a library, concepts of public/private do not apply, just put everything you need in FilterGraph. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH v2] fftools/ffmpeg_mux: fix reporting muxer EOF as error
Marton Balint (12023-04-24): > The change in ffmpeg.c "forces" muxers to check if they ever get AVERROR_EOF > for some real error condition and map them to e.g. AVERROR(EIO). And that is > an API change. Indeed. And the documentation agrees: * @return < 0 on error, = 0 if OK, 1 if flushed and there is no more data to flush * * @see av_interleaved_write_frame() */ int av_write_frame(AVFormatContext *s, AVPacket *pkt); * @return 0 on success, a negative AVERROR on error. * * @see av_write_frame(), AVFormatContext.max_interleave_delta */ int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt); No mention of EOF at all. Contrast with: * @return 0 if OK, < 0 on error or end of file. On error, pkt will be blank * (as if it came from av_packet_alloc()). … int av_read_frame(AVFormatContext *s, AVPacket *pkt); When EOF is intended, it written in the documentation. As it is, we should av_assert0(ret != AVERROR_EOF) in av_interleaved_write_frame() and fix the possible failures. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH v2] fftools/ffmpeg_mux: fix reporting muxer EOF as error
Marton Balint (12023-04-23): > https://ffmpeg.org/pipermail/ffmpeg-devel/2023-February/306247.html Looks like something that sould return AVERORR(EPIPE). Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 3/3] avfilter/vf_uspp: 1000% faster with threads
Michael Niedermayer (12023-03-16): > Subject: Re: [FFmpeg-devel] [PATCH 3/3] avfilter/vf_uspp: 1000% faster with > threads Rule of thumb: do not use variation percentages above +100% or below -50%, use ratios instead: avfilter/vf_uspp: 11 times faster with threads (11, really? not 10?) Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg: reset pointer, which could be reused by Android and iOS
"zhilizhao(赵志立)" (12023-03-03): > > av_freep(_files); > > av_freep(_files); > > +input_files = NULL; > > +output_files = NULL; > Until we decided to make ffmpeg cmd works as a library, > it doesn’t matter. It is worse than that: the patch completely ignores the semantic of av_freep(). Useless patch with no explanations at all in the mail => just ignore the mail. > I’m interested on the idea to make fftools > work like a library. But it depends on the community. That would be more or less what an extended libavfilter with movie source and sink would be. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] libavformat/tcp: add local_addr/local_port for network option
Rémi Denis-Courmont (12023-03-02): > That needs to match the socket protocol family, or bind() will fail. Oh, right! The proper way to work with getaddrinfo is to create the socket afterwards based on its return, but staying within a family is a good second best choice. Thanks for noticing. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] libavformat/tcp: add local_addr/local_port for network option
jack (12023-02-28):
> Signed-off-by: jack
> ---
> libavformat/tcp.c | 44
> 1 file changed, 44 insertions(+)
Thanks for the patch
You neglected to update the documentation, and a few remarks below.
>
> diff --git a/libavformat/tcp.c b/libavformat/tcp.c
> index a11ccbb913..598d61067e 100644
> --- a/libavformat/tcp.c
> +++ b/libavformat/tcp.c
> @@ -36,6 +36,8 @@ typedef struct TCPContext {
> const AVClass *class;
> int fd;
> int listen;
> +int local_port;
> +char *local_addr;
> int open_timeout;
> int rw_timeout;
> int listen_timeout;
> @@ -52,6 +54,8 @@ typedef struct TCPContext {
> #define E AV_OPT_FLAG_ENCODING_PARAM
> static const AVOption options[] = {
> { "listen", "Listen for incoming connections", OFFSET(listen),
> AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 2, .flags = D|E },
> +{ "local_port", "Local port",
> OFFSET(local_port), AV_OPT_TYPE_INT,{ .i64 = -1 }, -1,
> INT_MAX, .flags = D|E },
> +{ "local_addr", "Local address",
> OFFSET(local_addr), AV_OPT_TYPE_STRING, { .str = NULL }, 0,
> 0, .flags = D|E },
> { "timeout", "set timeout (in microseconds) of socket I/O
> operations", OFFSET(rw_timeout), AV_OPT_TYPE_INT, { .i64 = -1 },
> -1, INT_MAX, .flags = D|E },
> { "listen_timeout", "Connection awaiting timeout (in milliseconds)",
> OFFSET(listen_timeout), AV_OPT_TYPE_INT, { .i64 = -1 }, -1,
> INT_MAX, .flags = D|E },
> { "send_buffer_size", "Socket send buffer size (in bytes)",
> OFFSET(send_buffer_size), AV_OPT_TYPE_INT, { .i64 = -1 }, -1,
> INT_MAX, .flags = D|E },
> @@ -73,6 +77,39 @@ static const AVClass tcp_class = {
> static void customize_fd(void *ctx, int fd)
> {
> TCPContext *s = ctx;
> +
> +if (s->local_addr) {
s->local_addr || s->local_port
> +struct addrinfo hints = { 0 }, *ai;
> +int ret;
> +int port = 0;
> +
> +hints.ai_family = AF_UNSPEC;
> +hints.ai_socktype = SOCK_STREAM;
> +
> +if (s->local_port > 0)
> +port = s->local_port;
> +
> +ret = getaddrinfo(s->local_addr, "0", , );
The provided port should be used instead of this "0", and be a string,
and named service.
(The remote port should be a string too, alas.)
> +if (ret) {
> +av_log(ctx, AV_LOG_WARNING,
> + "Failed to bind local addr: %s port: %d err: %s\n",
> +s->local_addr, s->local_port, gai_strerror(ret));
Should be an error: failing to bind on the specified address can be a
security issue. And the error message is misleading.
> +} else {
> +if (ai->ai_family == AF_INET6) {
> +struct sockaddr_in6 * sockaddr_v6 = (struct sockaddr_in6
> *)ai->ai_addr;
> +sockaddr_v6->sin6_port = htons(port);
> +} else {
> +struct sockaddr_in * sockaddr = (struct sockaddr_in
> *)ai->ai_addr;
> +sockaddr->sin_port = htons(port);
> +}
Do not distinguish between protocols: ai_addr already contains all that
is needed.
> +ret = bind(fd, (struct sockaddr *)ai->ai_addr,
> (int)ai->ai_addrlen);
> +if (ret) {
> +av_log(ctx, AV_LOG_WARNING,
> +"Failed to bind local addr: %s port: %d err: %s\n",
> +s->local_addr, s->local_port, gai_strerror(ret));
> +}
You should be looping over the returned addresses.
> +}
> +}
> /* Set the socket's send or receive buffer sizes, if specified.
> If unspecified or setting fails, system default is used. */
> if (s->recv_buffer_size > 0) {
> @@ -129,6 +166,13 @@ static int tcp_open(URLContext *h, const char *uri, int
> flags)
> if (buf == endptr)
> s->listen = 1;
> }
> +if (av_find_info_tag(buf, sizeof(buf), "local_port", p)) {
> +s->local_port = strtol(buf, NULL, 10);
> +}
> +if (av_find_info_tag(buf, sizeof(buf), "local_addr", p)) {
> +av_freep(>local_addr);
> +s->local_addr = av_strndup(buf, strlen(buf));
> +}
> if (av_find_info_tag(buf, sizeof(buf), "timeout", p)) {
> s->rw_timeout = strtol(buf, NULL, 10);
> }
Regards,
--
Nicolas George
___
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".
Re: [FFmpeg-devel] [PATCH 2/3] lavc/assenc: set AV_CODEC_CAP_SINGLE_SUB_RECT
Ridley Combs (12023-02-21): > Fair enough, if we're fine with breaking the existing case further. > Should I simply drop rectangles after a first, or return an error? You have to ask? Between reporting an error and silently corrupting data, the answer is never in doubt. > This is only true for audio/video encoders; subtitle encoders still > use a different API, which does not have M:N support. There's some > long-ongoing work to change that, but for now, this seems like the > only way to deal with this case before that API overhaul. Oh, I had forgotten this. In that cas, I would suggest to keep API changes to a minimum: - Have assenc print and return an error if there are several rectangles. - Add a special case in the command-line tool ffmpeg based on the encoding codec. Or probably even better, for all text subtitles. This will not interfere with overhauling the subtitles encoding API. Of course, other developers might disagree, give it a few days. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH 2/3] lavc/assenc: set AV_CODEC_CAP_SINGLE_SUB_RECT
rcombs (12023-02-20): > This already gave garbled output when multiple rects were present, > so this is simply documenting an existing requirement. > --- > libavcodec/assenc.c | 2 ++ > 1 file changed, 2 insertions(+) NAK: the code has provisions for multiple rectangles, if you enforce a single rectangle you need to remove the code that is now useless. But I do not think pushing the issue onto the applications is a good way to fix the problem. Or even pushing the issue onto the framework, since the framework does not know the specifics. Better fix the code in ASS that handles multiple rectangles than inventing yet another annoying flag. A single frame can be encoded into multiple packets, so that should not be hard. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH 3/3] ffmpeg: respect AV_CODEC_CAP_SINGLE_SUB_RECT
rcombs (12023-02-20):
> Fixes ASS output when multiple rects are present.
> ---
> fftools/ffmpeg.c | 28 ++--
> 1 file changed, 18 insertions(+), 10 deletions(-)
Does this not belong to the framework?
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 9884e0c6c6..23eac52438 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -1021,6 +1021,7 @@ static void do_subtitle_out(OutputFile *of,
> AVCodecContext *enc;
> AVPacket *pkt = ost->pkt;
> int64_t pts;
> +int single_rect;
>
> if (sub->pts == AV_NOPTS_VALUE) {
> av_log(ost, AV_LOG_ERROR, "Subtitle packets must have a pts\n");
> @@ -1031,11 +1032,15 @@ static void do_subtitle_out(OutputFile *of,
>
> enc = ost->enc_ctx;
>
> +single_rect = !!(enc->codec->capabilities &
> AV_CODEC_CAP_SINGLE_SUB_RECT);
> +
> /* Note: DVB subtitle need one packet to draw them and one other
> packet to clear them */
> /* XXX: signal it in the codec context ? */
> if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE)
> nb = 2;
> +else if (single_rect)
> +nb = FFMAX(sub->num_rects, 1);
> else
> nb = 1;
>
> @@ -1044,7 +1049,7 @@ static void do_subtitle_out(OutputFile *of,
> if (output_files[ost->file_index]->start_time != AV_NOPTS_VALUE)
> pts -= output_files[ost->file_index]->start_time;
> for (i = 0; i < nb; i++) {
> -unsigned save_num_rects = sub->num_rects;
> +AVSubtitle local_sub = *sub;
>
> if (!check_recording_time(ost, pts, AV_TIME_BASE_Q))
> return;
> @@ -1053,19 +1058,22 @@ static void do_subtitle_out(OutputFile *of,
> if (ret < 0)
> report_and_exit(AVERROR(ENOMEM));
>
> -sub->pts = pts;
> +local_sub.pts = pts;
> // start_display_time is required to be 0
> -sub->pts += av_rescale_q(sub->start_display_time,
> (AVRational){ 1, 1000 }, AV_TIME_BASE_Q);
> -sub->end_display_time -= sub->start_display_time;
> -sub->start_display_time = 0;
> -if (i == 1)
> -sub->num_rects = 0;
> +local_sub.pts += av_rescale_q(sub->start_display_time,
> (AVRational){ 1, 1000 }, AV_TIME_BASE_Q);
> +local_sub.end_display_time -= sub->start_display_time;
> +local_sub.start_display_time = 0;
> +
> +if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE && i == 1)
> +local_sub.num_rects = 0;
> +else if (single_rect && sub->num_rects > 0) {
> +local_sub.num_rects = 1;
> +local_sub.rects += i;
> +}
>
> ost->frames_encoded++;
>
> -subtitle_out_size = avcodec_encode_subtitle(enc, pkt->data,
> pkt->size, sub);
> -if (i == 1)
> -sub->num_rects = save_num_rects;
> +subtitle_out_size = avcodec_encode_subtitle(enc, pkt->data,
> pkt->size, _sub);
> if (subtitle_out_size < 0) {
> av_log(ost, AV_LOG_FATAL, "Subtitle encoding failed\n");
> exit_program(1);
Regards,
--
Nicolas George
___
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".
Re: [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT
rcombs (12023-02-20): > --- > doc/APIchanges | 3 +++ > libavcodec/codec.h | 5 + > libavcodec/version.h | 2 +- > 3 files changed, 9 insertions(+), 1 deletion(-) And... No change to the framework to make use of this flag? Like, return AVERROR(EINVAL) if the flag is present but several rectangles are given? Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] tests: actually test yadif's 10 and 16-bit functions
James Darnley (12023-02-20): > --- > tests/fate/filter-video.mak | 4 +-- > tests/ref/fate/filter-yadif10 | 60 +-- > tests/ref/fate/filter-yadif16 | 60 +-- > 3 files changed, 62 insertions(+), 62 deletions(-) > > diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak > index 63873a7a07..65965d8518 100644 > --- a/tests/fate/filter-video.mak > +++ b/tests/fate/filter-video.mak > @@ -16,8 +16,8 @@ fate-filter-yadif-mode0: CMD = framecrc -flags bitexact > -idct simple -i $(TARGET > fate-filter-yadif-mode1: CMD = framecrc -flags bitexact -idct simple -i > $(TARGET_SAMPLES)/mpeg2/mpeg2_field_encoding.ts -frames:v 59 -vf yadif=1 > > FATE_YADIF-$(call FILTERDEMDEC, YADIF SCALE, MPEGTS, MPEG2VIDEO) += > fate-filter-yadif10 fate-filter-yadif16 > -fate-filter-yadif10: CMD = framecrc -flags bitexact -idct simple -i > $(TARGET_SAMPLES)/mpeg2/mpeg2_field_encoding.ts -flags bitexact -pix_fmt > yuv420p10le -frames:v 30 -vf yadif=0,scale > -fate-filter-yadif16: CMD = framecrc -flags bitexact -idct simple -i > $(TARGET_SAMPLES)/mpeg2/mpeg2_field_encoding.ts -flags bitexact -pix_fmt > yuv420p16le -frames:v 30 -vf yadif=0,scale > +fate-filter-yadif10: CMD = framecrc -flags bitexact -idct simple -i > $(TARGET_SAMPLES)/mpeg2/mpeg2_field_encoding.ts -flags bitexact -pix_fmt > yuv420p10le -frames:v 30 -vf scale,format=yuv420p10le,yadif=0 > +fate-filter-yadif16: CMD = framecrc -flags bitexact -idct simple -i > $(TARGET_SAMPLES)/mpeg2/mpeg2_field_encoding.ts -flags bitexact -pix_fmt > yuv420p16le -frames:v 30 -vf scale,format=yuv420p16le,yadif=0 Moving scale before yadif is right, but format= is redundant with -pix_fmt. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH 1/2] lavfi/Makefile: Dont compile unused files.
Hi. Matt Oliver (12023-01-15): > vf_nlmeans and vf_atadenoisedont contain any code on 32bit x86 so dont > build them. Hi This sentence seems to be missing a few words. > > --- > libavfilter/x86/Makefile | 4 > 1 file changed, 4 insertions(+) > > diff --git a/libavfilter/x86/Makefile b/libavfilter/x86/Makefile > index e87481bd7a..9a68b9204b 100644 > --- a/libavfilter/x86/Makefile > +++ b/libavfilter/x86/Makefile > @@ -44,7 +44,9 @@ X86ASM-OBJS-$(CONFIG_SCENE_SAD) += > x86/scene_sad.o > > X86ASM-OBJS-$(CONFIG_AFIR_FILTER)+= x86/af_afir.o > X86ASM-OBJS-$(CONFIG_ANLMDN_FILTER) += x86/af_anlmdn.o > +ifdef ARCH_X86_64 > X86ASM-OBJS-$(CONFIG_ATADENOISE_FILTER) += x86/vf_atadenoise.o > +endif Do you mean the file is useless yet configure set CONFIG_ATADENOISE_FILTER? That looks suspicious. The same goes for the other place and the other patch. > X86ASM-OBJS-$(CONFIG_BLEND_FILTER) += x86/vf_blend.o > X86ASM-OBJS-$(CONFIG_BWDIF_FILTER) += x86/vf_bwdif.o > X86ASM-OBJS-$(CONFIG_COLORSPACE_FILTER) += x86/colorspacedsp.o > @@ -62,7 +64,9 @@ X86ASM-OBJS-$(CONFIG_LIMITER_FILTER) += > x86/vf_limiter.o > X86ASM-OBJS-$(CONFIG_LUT3D_FILTER) += x86/vf_lut3d.o > X86ASM-OBJS-$(CONFIG_MASKEDCLAMP_FILTER) += x86/vf_maskedclamp.o > X86ASM-OBJS-$(CONFIG_MASKEDMERGE_FILTER) += x86/vf_maskedmerge.o > +ifdef ARCH_X86_64 > X86ASM-OBJS-$(CONFIG_NLMEANS_FILTER) += x86/vf_nlmeans.o > +endif > X86ASM-OBJS-$(CONFIG_OVERLAY_FILTER) += x86/vf_overlay.o > X86ASM-OBJS-$(CONFIG_PP7_FILTER) += x86/vf_pp7.o > X86ASM-OBJS-$(CONFIG_PSNR_FILTER)+= x86/vf_psnr.o Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 1/2] doc/dev_community: add the addresses of the committees
Thilo Borgman (12023-02-08): > From: Nicolas George > > Omitting the .org from the address should be protection enough > against spam spiders. > > Signed-off-by: Nicolas George > --- > doc/dev_community/community.md | 4 > 1 file changed, 4 insertions(+) Thanks for picking this up. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] avdevice: add dxgigrab
Timo Rothenpieler (12023-02-09): > How do you get access to the d3d hwdevice, given this lives in lavd, which > has no provisions for that? Would it work better if it was in the same library? Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] avdevice/xcbgrab: enable window resizing
aline.gondimsan...@savoirfairelinux.com (12023-02-09): > From: Aline Gondim Santos > > Signed-off-by: Aline Gondim Santos > --- > libavdevice/xcbgrab.c | 180 +- > 1 file changed, 39 insertions(+), 141 deletions(-) Hi. Thanks for the patch. Are you removing support for using shared memory entirely to implement this feature? If so, can we see some benchmarks comparisons? Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] avfilter: use ff_inlink_make_frame_writable()
av_frame_clone(in);
> -if (!new)
> +if (!new) {
> +av_frame_free();
> return AVERROR(ENOMEM);
> +}
>
> ret = ff_filter_frame(outlink, new);
>
> if (in->repeat_pict) {
> -av_frame_make_writable(out);
> +ret = ff_inlink_make_frame_writable(inlink, );
> +if (ret < 0) {
> +av_frame_free();
> +return ret;
> +}
> update_pts(outlink, out, in->pts, 2);
> for (i = 0; i < s->nb_planes; i++) {
> av_image_copy_plane(out->data[i], out->linesize[i] * 2,
> @@ -121,7 +131,11 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *in) {
> }
> } else {
> for (i = 0; i < s->nb_planes; i++) {
> -av_frame_make_writable(out);
> +ret = ff_inlink_make_frame_writable(inlink, );
> +if (ret < 0) {
> +av_frame_free();
> +return ret;
> +}
> av_image_copy_plane(out->data[i] + out->linesize[i],
> out->linesize[i] * 2,
> in->data[i] + in->linesize[i],
> in->linesize[i] * 2,
> s->linesize[i], s->planeheight[i] / 2);
> @@ -133,13 +147,19 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *in) {
> AVFrame *new;
>
> new = av_frame_clone(in);
> -if (!new)
> +if (!new) {
> +av_frame_free();
> return AVERROR(ENOMEM);
> +}
>
> ret = ff_filter_frame(outlink, new);
> state = 0;
> } else {
> -av_frame_make_writable(out);
> +ret = ff_inlink_make_frame_writable(inlink, );
> +if (ret < 0) {
> +av_frame_free();
> +return ret;
> +}
> update_pts(outlink, out, in->pts, 1);
> for (i = 0; i < s->nb_planes; i++) {
> av_image_copy_plane(out->data[i], out->linesize[i] * 2,
> diff --git a/libavfilter/vf_signalstats.c b/libavfilter/vf_signalstats.c
> index e6f84be9ba..09a93010da 100644
> --- a/libavfilter/vf_signalstats.c
> +++ b/libavfilter/vf_signalstats.c
> @@ -23,6 +23,7 @@
> #include "libavutil/intreadwrite.h"
> #include "libavutil/opt.h"
> #include "libavutil/pixdesc.h"
> +#include "filters.h"
> #include "internal.h"
>
> enum FilterMode {
> @@ -565,7 +566,7 @@ static int filter_frame8(AVFilterLink *link, AVFrame *in)
> int tothue = 0;
> int dify = 0, difu = 0, difv = 0;
> uint16_t masky = 0, masku = 0, maskv = 0;
> -
> +int ret;
> int filtot[FILT_NUMB] = {0};
> AVFrame *prev;
>
> @@ -588,7 +589,11 @@ static int filter_frame8(AVFilterLink *link, AVFrame *in)
>
> if (s->outfilter != FILTER_NONE) {
> out = av_frame_clone(in);
> -av_frame_make_writable(out);
> +ret = ff_inlink_make_frame_writable(link, );
> +if (ret < 0) {
> +av_frame_free();
> +return ret;
> +}
> }
>
> ff_filter_execute(ctx, compute_sat_hue_metrics8, _huesat,
> @@ -790,7 +795,7 @@ static int filter_frame16(AVFilterLink *link, AVFrame *in)
>
> int filtot[FILT_NUMB] = {0};
> AVFrame *prev;
> -
> +int ret;
> AVFrame *sat = s->frame_sat;
> AVFrame *hue = s->frame_hue;
> const uint16_t *p_sat = (uint16_t *)sat->data[0];
> @@ -810,7 +815,11 @@ static int filter_frame16(AVFilterLink *link, AVFrame
> *in)
>
> if (s->outfilter != FILTER_NONE) {
> out = av_frame_clone(in);
> -av_frame_make_writable(out);
> +ret = ff_inlink_make_frame_writable(link, );
> +if (ret < 0) {
> +av_frame_free();
> +return ret;
> +}
> }
>
> ff_filter_execute(ctx, compute_sat_hue_metrics16, _huesat,
> diff --git a/libavfilter/vf_telecine.c b/libavfilter/vf_telecine.c
> index e8de63bbcf..227de6f733 100644
> --- a/libavfilter/vf_telecine.c
> +++ b/libavfilter/vf_telecine.c
> @@ -29,6 +29,7 @@
> #include "libavutil/opt.h"
> #include "libavutil/pixdesc.h"
> #include "avfilter.h"
> +#include "filters.h"
> #include "formats.h"
> #include "internal.h"
> #include "video.h"
> @@ -182,7 +183,11 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *inpicref)
> }
>
> if (s->occupied) {
> -av_frame_make_writable(s->frame[nout]);
> +ret = ff_inlink_make_frame_writable(inlink, >frame[nout]);
> +if (ret < 0) {
> +av_frame_free();
> +return ret;
> +}
> for (i = 0; i < s->nb_planes; i++) {
> // fill in the EARLIER field from the buffered pic
> av_image_copy_plane(s->frame[nout]->data[i] +
> s->frame[nout]->linesize[i] * s->first_field,
> @@ -208,7 +213,11 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *inpicref)
>
> while (len >= 2) {
> // output THIS image as-is
> -av_frame_make_writable(s->frame[nout]);
> +ret = ff_inlink_make_frame_writable(inlink, >frame[nout]);
> +if (ret < 0) {
> +av_frame_free();
> +return ret;
> +}
> for (i = 0; i < s->nb_planes; i++)
> av_image_copy_plane(s->frame[nout]->data[i],
> s->frame[nout]->linesize[i],
> inpicref->data[i], inpicref->linesize[i],
> diff --git a/libavfilter/vf_vidstabdetect.c b/libavfilter/vf_vidstabdetect.c
> index 62b998e171..b27b1e40a6 100644
> --- a/libavfilter/vf_vidstabdetect.c
> +++ b/libavfilter/vf_vidstabdetect.c
> @@ -27,6 +27,7 @@
> #include "libavutil/opt.h"
> #include "libavutil/pixdesc.h"
> #include "avfilter.h"
> +#include "filters.h"
> #include "internal.h"
>
> #include "vidstabutils.h"
> @@ -149,10 +150,15 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *in)
>
> AVFilterLink *outlink = inlink->dst->outputs[0];
> VSFrame frame;
> -int plane;
> +int plane, ret;
>
> -if (s->conf.show > 0 && !av_frame_is_writable(in))
> -av_frame_make_writable(in);
> +if (s->conf.show > 0 && !av_frame_is_writable(in)) {
> +ret = ff_inlink_make_frame_writable(inlink, );
> +if (ret < 0) {
> +av_frame_free();
> +return ret;
> +}
> +}
>
> for (plane = 0; plane < md->fi.planes; plane++) {
> frame.data[plane] = in->data[plane];
No other remark from me, thanks.
Regards,
--
Nicolas George
___
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".
Re: [FFmpeg-devel] [PATCH] avfilter: use ff_inlink_make_frame_writable()
av_frame_make_writable(out);
> +ret = ff_inlink_make_frame_writable(inlink, );
> +if (ret < 0)
> +return ret;
Ditto?
> update_pts(outlink, out, in->pts, 1);
> for (i = 0; i < s->nb_planes; i++) {
> av_image_copy_plane(out->data[i], out->linesize[i] * 2,
> diff --git a/libavfilter/vf_signalstats.c b/libavfilter/vf_signalstats.c
> index e6f84be9ba..93ff378b31 100644
> --- a/libavfilter/vf_signalstats.c
> +++ b/libavfilter/vf_signalstats.c
> @@ -23,6 +23,7 @@
> #include "libavutil/intreadwrite.h"
> #include "libavutil/opt.h"
> #include "libavutil/pixdesc.h"
> +#include "filters.h"
> #include "internal.h"
>
> enum FilterMode {
> @@ -565,7 +566,7 @@ static int filter_frame8(AVFilterLink *link, AVFrame *in)
> int tothue = 0;
> int dify = 0, difu = 0, difv = 0;
> uint16_t masky = 0, masku = 0, maskv = 0;
> -
> +int ret;
> int filtot[FILT_NUMB] = {0};
> AVFrame *prev;
>
> @@ -588,7 +589,9 @@ static int filter_frame8(AVFilterLink *link, AVFrame *in)
>
> if (s->outfilter != FILTER_NONE) {
> out = av_frame_clone(in);
> -av_frame_make_writable(out);
> +ret = ff_inlink_make_frame_writable(link, );
> +if (ret < 0)
> +return ret;
Ditto?
> }
>
> ff_filter_execute(ctx, compute_sat_hue_metrics8, _huesat,
> @@ -790,7 +793,7 @@ static int filter_frame16(AVFilterLink *link, AVFrame *in)
>
> int filtot[FILT_NUMB] = {0};
> AVFrame *prev;
> -
> +int ret;
> AVFrame *sat = s->frame_sat;
> AVFrame *hue = s->frame_hue;
> const uint16_t *p_sat = (uint16_t *)sat->data[0];
> @@ -810,7 +813,9 @@ static int filter_frame16(AVFilterLink *link, AVFrame *in)
>
> if (s->outfilter != FILTER_NONE) {
> out = av_frame_clone(in);
> -av_frame_make_writable(out);
> +ret = ff_inlink_make_frame_writable(link, );
> +if (ret < 0)
> +return ret;
Ditto?
> }
>
> ff_filter_execute(ctx, compute_sat_hue_metrics16, _huesat,
> diff --git a/libavfilter/vf_telecine.c b/libavfilter/vf_telecine.c
> index e8de63bbcf..227de6f733 100644
> --- a/libavfilter/vf_telecine.c
> +++ b/libavfilter/vf_telecine.c
> @@ -29,6 +29,7 @@
> #include "libavutil/opt.h"
> #include "libavutil/pixdesc.h"
> #include "avfilter.h"
> +#include "filters.h"
> #include "formats.h"
> #include "internal.h"
> #include "video.h"
> @@ -182,7 +183,11 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *inpicref)
> }
>
> if (s->occupied) {
> -av_frame_make_writable(s->frame[nout]);
> +ret = ff_inlink_make_frame_writable(inlink, >frame[nout]);
> +if (ret < 0) {
> +av_frame_free();
> +return ret;
> +}
> for (i = 0; i < s->nb_planes; i++) {
> // fill in the EARLIER field from the buffered pic
> av_image_copy_plane(s->frame[nout]->data[i] +
> s->frame[nout]->linesize[i] * s->first_field,
> @@ -208,7 +213,11 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *inpicref)
>
> while (len >= 2) {
> // output THIS image as-is
> -av_frame_make_writable(s->frame[nout]);
> +ret = ff_inlink_make_frame_writable(inlink, >frame[nout]);
> +if (ret < 0) {
> +av_frame_free();
> +return ret;
> +}
> for (i = 0; i < s->nb_planes; i++)
> av_image_copy_plane(s->frame[nout]->data[i],
> s->frame[nout]->linesize[i],
> inpicref->data[i], inpicref->linesize[i],
> diff --git a/libavfilter/vf_vidstabdetect.c b/libavfilter/vf_vidstabdetect.c
> index 62b998e171..3eb1abab46 100644
> --- a/libavfilter/vf_vidstabdetect.c
> +++ b/libavfilter/vf_vidstabdetect.c
> @@ -27,6 +27,7 @@
> #include "libavutil/opt.h"
> #include "libavutil/pixdesc.h"
> #include "avfilter.h"
> +#include "filters.h"
> #include "internal.h"
>
> #include "vidstabutils.h"
> @@ -149,10 +150,13 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *in)
>
> AVFilterLink *outlink = inlink->dst->outputs[0];
> VSFrame frame;
> -int plane;
> +int plane, ret;
>
> -if (s->conf.show > 0 && !av_frame_is_writable(in))
> -av_frame_make_writable(in);
> +if (s->conf.show > 0 && !av_frame_is_writable(in)) {
> +ret = ff_inlink_make_frame_writable(inlink, );
> +if (ret < 0)
> +return ret;
> +}
>
> for (plane = 0; plane < md->fi.planes; plane++) {
> frame.data[plane] = in->data[plane];
No objections except for the possible leaks.
--
Nicolas George
___
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".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Andreas Rheinhardt (12023-02-01):
> "One special guarantee is made in order to simplify the use of unions:
> if a union contains several structures that share a common initial
> sequence (see below), and if the union object currently contains one of
> these structures, it is permitted to inspect the common initial part of
> any of them anywhere that a declaration of the completed type of the
> union is visible. Two structures share a common initial sequence if
> corresponding members have compatible types (and, for bit-fields, the
> same widths) for a sequence of one or more initial members."
>
> But there just is no union between the FF_INTERNAL_FIELDS
> !defined(FF_INTERNAL_FIELDS) structures in the whole codebase.
It is not necessary that there is one: it is enough that there could be
one in another compilation unit, the code that handles these structures
does not know what exists in the rest of the code base. This guarantee
was made FOR unions, but it does not REQUIRE unions, it applies to
anything that is semantically equivalent to a union.
> Furthermore, said guarantee is only for inspecting, i.e. reading. For
> example, for the following two structs sharing a common initial sequence,
>
> struct Small {
> uint64_t foo;
> uint32_t bar;
> };
> struct Big {
> uint64_t foo;
> uint32_t bar;
> uint32_t baz;
> };
>
> if one had a union { struct Small; struct Big; }, a write to Small.bar
> may clobber Big.baz.
That is a good point. It does not apply when the first field of Big is
Small itself, which is the case that I absolutely know is supported,
because in that case Big will embed the padding of Small. I had not
thought of this.
Fortunately, even if we are not 100% in the case that is officially
supported, there are multiple reasons that each should be enough to
guarantee that our code will work:
- Between the public part of the structure and the #ifede
FF_INTERNAL_FIELDS, there are a lot of "not part of the public API"
fields", changing ch_layout will not clobber incfg because incfg is
known at this point.
- Even if there was no fields in between, reserved[] offers the same
protection.
- And even if there was no gap, we are good because applications, and
even filters, are not supposed to modify AVFilterLink, only the
framework and the framework knows the whole structure.
In fact, that last remark makes me think of another solution, for the
slightly longer term: make AVFilterLink entirely opaque. The only
documented reason for application to access AVFilterLink was to get the
parameters of buffersink, but buffersink has a real API to get this
since 2016.
Anyway, if you prefer using a compilation option, I have no objection.
My only concern is that when a developer writes:
if (link->x == ... &&
link->y == ... &&
link->status_in == ... &&
link->min_samples == ..)
they have to remember that no, status_in is different from all the
others.
And it is especially bad in this particular case because the distinction
is not public / private, which makes some semantic sense and might be
easier to remember, the distinction is actually
public-or-private-because-of-a-comment / private-because-of-a-ifdef.
But even if the distinction really was public / private, I consider it
unacceptable if we can do otherwise. And we can.
Regards,
--
Nicolas George
___
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".
Re: [FFmpeg-devel] [PATCH] Waveform Archiver decoder and demuxer
Tomas Härdin (12023-02-01): > This belongs in the demuxer. In fact it appears the decoder duplicates > code from the demuxer. Why? Because sharing code between demuxer and decoder is extremely annoying, because somebody decided the libraries must be separate even though all the technical reasons for it have less annoying solutions. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Andreas Rheinhardt (12023-02-01): > PS: Upon rethinking, it is not only b) that contains undefined > behaviour; it is b)-d) as well as the current code. The reason is that > the type of AVFilterLink as seen in the files with FF_INTERNAL_FIELDS is > not compatible with the type of AVFilterLink from the files without > FF_INTERNAL_FIELDS. This also makes derived types, like the types of > function declarations containing pointers to AVFilterLink incompatible > and this is a violation of 6.2.7(2) of C11. I wonder if this will become > a real problem with lto someday. No: read the second half of the previous paragraph: two structures with common first fields are compatible types. What we have been using is a deliberately supported construct. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Andreas Rheinhardt (12023-01-31):
> Details please.
I was not going to spend time explaining unless I had assurance it was
because the issue of requiring changes in the internal implementation
and developers habits just to hide some fields was taken into account.
Coming from somebody else, I would have expected the question to be only
to find the first excuses to shoot the proposal down.
But you more or less found the same ideas I did anyway.
> I can only think of the following:
> a) Allow the use of -fms-extensions. This allows structs with tags and
> typedefs thereof to be used as unnamed struct members with the members
> of the unnamed structure being treated as members of the enclosing
> structure. One could then use a pointer to the big internal structure
> internally and one does not need to remember whether a field is internal
> or not. There is still a problem, though: One needs to cast from
> AVFilterContext.(inputs|outputs). This should be done via dedicated
> inlined getters, but this is a bit more typing. E.g.
> "input_from_ctx(ctx, i)" instead of "ctx->inputs[i]". Of course, it
> might also be shorter if someone has a short name.
> GCC, Clang, MSVC and the IIRC the intel compilers support this.
>
> b) Add a big #define AVFILTERLINK in avfilter.h that expands to the
> public part of AVFilterLink and change the declaration of AVFilterLink
> to "struct AVFilterLink { AVFILTERLINK };" and use declare the internal
> struct via
> "struct FilterLinkInternal {
> AVFILTERLINK
> /* actual internal fields */
> };"
> This has the downside of actually being an aliasing violation and of
> adding considerable ugliness to avfilter.h, in particular during
> deprecations (like with FF_API_OLD_CHANNEL_LAYOUT -- you can't check via
> #if in the implementation of a macro). I also don't know whether it
> plays nicely with tools that deal with source code annotations.
Or use a “#include "avfilter_link_internal_fields.h" instead of a macro.
> c) Wrap the internal part in an #ifdef HAVE_AV_CONFIG_H, optionally
> using #if defined(HAVE_AV_CONFIG_H) && defined(BUILDING_avfilter).
> d) Same as c), but strip this stuff from installed headers.
>
> I consider b)-d) as inferior to a), which I consider superior to Anton's
> proposal, but the big drawback is its reliance on a compiler extension.
Once and for all: If the solution requires changing things in the
internal implementation C files and changing the habits of the people
who work on these files (that goes together), then it is inferior to a
solution that does not require that.
Your solution (a) has this flaw, plus relies on an extension that is
probably not available on all supported platforms (Microsoft I am
looking at you).
My favor goes to (d). Probably with a stricter test for the (c) part
because the internal fields are not necessary for the filters and
therefore could be hidden there too.
Regards,
--
Nicolas George
signature.asc
Description: PGP signature
___
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".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Nicolas George (12023-01-31): > > * it prevents filterlink internals from being visible in a > > public header, where they have no business being > > * it is a step towards hiding more of lavfi internals from public > > headers > > * the same pattern is already and ever more widely used in the other Note to the TC who will decide: I do not oppose the efforts mentioned in these two points (that are actually the same points twice), I only oppose this particular solution because of this drawback: > * It requires the developers to remember which field is public and which > field is private, which is not something relevant here (is is relevant > elsewhere). Without looking very far, I can think of several different ways of hiding the internal fields better without requiring changes to the implementation. I would not oppose such a change. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Anton Khirnov (12023-01-31): > I still see no objective facts supporting your claim of exclusive > maintainership over the entirety of lavfi generic code and public API. The fact is very simple: I am the only one who understand how this code works. > So to avoid any further pointless bickering, I'm hereby asking the TC to > resolve this. Just so we are clear: you are a party in this, you can therefore not be a judge. > To summarize my view, this patch is an improvement because: > * it prevents filterlink internals from being visible in a > public header, where they have no business being > * it is a step towards hiding more of lavfi internals from public > headers > * the same pattern is already and ever more widely used in the other > libraries and ffmpeg CLI > * it is supported by Andreas (who submitted a more general analogue of > this patch over a year ago) and Paul > * I am not aware of anyone other than Nicolas being against it It is a worsening because: * It requires the developers to remember which field is public and which field is private, which is not something relevant here (is is relevant elsewhere). Of course, if you think about it two seconds, you realize it affects the person who knows the code very well and used/wants to work on it intensively more than the developers who move from one part of the code to another and have to re-learn everything. But thinking two seconds how your changes affects other people does not seem like your forte either. Therefore, I add this point: * If this change is applied, FFmpeg needs to find somebody else to maintain the core of libavfilter. (And I predict you will not.) -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Paul B Mahol (12023-01-31): > No, you do not. Calling your libavfilter framework code "complex" is disgrace > to really complex code in non-framework part of libavfilter or else in > FFmpeg libraries. > > libavfilter framework needs serious overhaul. > It have numerous limitations and other stuff too much exposed to user, > that really should not be. > > You do not maintain code at all, you just block patches and never > contribute anymore anything useful besides blocking patches. … says the person who does not know that the duration between pts=3 and pts=5 must be 2. This is a bad joke. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Anton Khirnov (12023-01-31): > Do you? > > You keep implying in your emails that you are the authority on lavfi, > but I see no objective support for this claim. MAINTAINERS only lists > you as the maintainer for graphdump.c and two filters. > And taken e.g. by the number of commits touching libavfilter/, mine is > similar to yours, and both are far behind other people. I know the code, how it works. I know how the code needs to evolve to achieve new features without breaking existing. I review and apply patches to fix bugs when they come; fortunately it does not happen frequently. > Maybe someone should replace those tricky bits with something simpler > then. Yes, do that, and I will laugh. The code is tricky because the needs are tricky. “Something simpler” is precisely what came from libav and I needed to fix a decade ago. > Ah yes, preventing one person from enforcing his opinion over those of > multiple other people is "authoritarianism". I don't think that word > means what you think it means. > > So take your pick - either stop opposing this patch or we have a vote > over it and see what other people think. Go ahead, have a vote. I no longer care. If it goes against me, that will make one less thing to consider my responsibility. We should never have asked ourselves “how should we change to entice forkers back”, we should have asked “how must the forkers change if they want to be accepted back”. You did not change. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Anton Khirnov (12023-01-31): > I find this a significant improvement in code quality, making it easier > to maintain. You can say that, you do not maintain it. > Making it obvious which field is private and which is public is a > feature, not a bug. Then I do not want this feature. Making people expend effort for no reason at all. > I'll also note that > - we've been switching to this precise pattern everywhere else in the > project Well, too bad. > - the most prolific lavfi contributor recently (Paul) has no objections > to this patch > - the second most prolific lavfi contributor recently (Andreas) told me > on IRC he planned to write this same patch himself It is not a matter of volume, it is a matter of complexity. Since Stefano is no longer involved in the coding, I am the only one who knows how the tricky bits of libavfilter work. > So if you insist on objecting to this patch, it seems to me that a vote > would be in order. Well, go ahead, but please be aware that a vote cannot force me to maintain that code. If this project goes over my objections “that patch makes my maintenance work harder”, then be very careful to involve in your plans “find somebody else willing to debug the code of libavfilter”. Good luck. In fact, I think I will take it very easy with maintenance duty until further notice. I am really fed up with the direction this project is taking. Authoritarianism, that was the other side of the fork, and we all know what happened there. And who is to blame. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH 4/5] lavfi/graphparser: reimplement avfilter_graph_parse* using new API
James Almer (12023-01-23): > What's the difference between a RFC and a "I have this idea, what do you > think of this mock up?" email? That if suggestions are made to improve or > change said approach, then at worst what happens is that the writer wasted > /his/ time writing 30k lines of which a fifth might need to be redone. You are assuming that somebody who already wrote 30K of code (not even lines) will be as ready to make changes that require rewriting 27K than somebody who did not already write the code. We had the exact problem with channel layouts: the structure should have been refcounted, but since the code was written without consulting people who would have to use it and neglected necessary features, it was not done, and since the code was already written the change was not seriously considered. Furthermore, communicating early would save time to somebody who might be at the early stages of working on the same thing. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH 3/5] lavfi: add a new filtergraph parsing API
N(nb_io, nb_l); i++)
> +if (!l[i] && io[i]->label && !strcmp(io[i]->label, label)) {
> +*pp = p;
> +return i;
> +}
> +}
> +
> +idx_filter = 0;
> +}
> +
> +*pp = NULL;
> +return 0;
> +}
If I read this code correctly, it will only find link labels that are
defined later in the chain or in a later chain. But referring to earlier
link labels is supported. Even cycles are meant to be supported at
parsing level (format negotiation will likely fail, though, but being
able to make cycles was a goal from the start).
> +
> +static int inout_add(AVFilterInOut **inouts, AVFilterContext *f, unsigned
> pad_idx,
> + const char *label)
> +{
> +AVFilterInOut *io = av_mallocz(sizeof(*io));
> +
> +if (!io)
> +return AVERROR(ENOMEM);
> +
> +io->filter_ctx = f;
> +io->pad_idx= pad_idx;
> +
> +if (label) {
> +io->name = av_strdup(label);
> +if (!io->name) {
> +avfilter_inout_free();
> +return AVERROR(ENOMEM);
> +}
> +}
> +
> +append_inout(inouts, );
> +
> +return 0;
> +}
> +
> +static int link_inputs(AVFilterGraphSegment *seg, size_t idx_chain,
> + size_t idx_filter, AVFilterInOut **inputs)
> +{
> +AVFilterChain *ch = seg->chains[idx_chain];
> +AVFilterParams *p = ch->filters[idx_filter];
> +AVFilterContext *f = p->filter;
> +
> +int ret;
> +
> +if (f->nb_inputs < p->nb_inputs) {
> +av_log(seg->graph, AV_LOG_ERROR,
> + "More input link labels specified for filter '%s' than "
> + "it has inputs: %u > %d\n", f->filter->name,
> + p->nb_inputs, f->nb_inputs);
> +return AVERROR(EINVAL);
> +}
> +
> +for (unsigned in = 0; in < f->nb_inputs; in++) {
> +const char *label = (in < p->nb_inputs) ? p->inputs[in]->label :
> NULL;
> +
> +// skip already linked inputs
> +if (f->inputs[in])
> +continue;
> +
> +if (label) {
> +AVFilterParams *po = NULL;
> +unsigned idx = find_linklabel(seg, label, 1, idx_chain,
> idx_filter, );
> +
> +if (po) {
> +ret = avfilter_link(po->filter, idx, f, in);
> +if (ret < 0)
> +return ret;
> +
> +continue;
> +}
> +}
> +
> +ret = inout_add(inputs, f, in, label);
> +if (ret < 0)
> +return ret;
> +}
> +
> +return 0;
> +}
> +
> +static int link_outputs(AVFilterGraphSegment *seg, size_t idx_chain,
> +size_t idx_filter, AVFilterInOut **outputs)
> +{
> +AVFilterChain *ch = seg->chains[idx_chain];
> +AVFilterParams *p = ch->filters[idx_filter];
> +AVFilterContext *f = p->filter;
> +
> +int ret;
> +
> +if (f->nb_outputs < p->nb_outputs) {
> +av_log(seg->graph, AV_LOG_ERROR,
> + "More output link labels specified for filter '%s' than "
> + "it has outputs: %u > %d\n", f->filter->name,
> + p->nb_outputs, f->nb_outputs);
> +return AVERROR(EINVAL);
> +}
> +for (unsigned out = 0; out < f->nb_outputs; out++) {
> +char *label = (out < p->nb_outputs) ? p->outputs[out]->label : NULL;
> +
> +// skip already linked outputs
> +if (f->outputs[out])
> +continue;
> +
> +if (label) {
> +AVFilterParams *po = NULL;
> +unsigned idx = find_linklabel(seg, label, 0, idx_chain,
> idx_filter, );
> +
> +if (po) {
> +ret = avfilter_link(f, out, po->filter, idx);
> +if (ret < 0)
> +return ret;
> +
> +continue;
> +}
> +}
> +
> +// if this output is unlabeled, try linking it to an unlabeled
> +// input in the next non-disabled filter in the chain
> +for (size_t i = idx_filter + 1; i < ch->nb_filters && !label; i++) {
> +AVFilterParams *p_next = ch->filters[i];
> +
> +if (!p_next->filter)
> +continue;
> +
> +for (unsigned in = 0; in < p_next->filter->nb_inputs; in++) {
> +if (!p_next->filter->inputs[in] &&
> +(in >= p_next->nb_inputs || !p_next->inputs[in]->label))
> {
> +ret = avfilter_link(f, out, p_next->filter, in);
> +if (ret < 0)
> +return ret;
> +
> +goto cont;
> +}
> +}
> +break;
> +}
> +
> +ret = inout_add(outputs, f, out, label);
> +if (ret < 0)
> +return ret;
> +
> +cont:;
> +}
> +
> +return 0;
> +}
> +
> +int avfilter_graph_segment_link(AVFilterGraphSegment *seg, int flags,
> +AVFilterInOut **inputs,
> +AVFilterInOut **outputs)
> +{
> +int ret;
> +
> +*inputs = NULL;
> +*outputs = NULL;
> +
> +if (flags)
> +return AVERROR(ENOSYS);
> +
> +for (size_t idx_chain = 0; idx_chain < seg->nb_chains; idx_chain++) {
> +AVFilterChain *ch = seg->chains[idx_chain];
> +
> +for (size_t idx_filter = 0; idx_filter < ch->nb_filters;
> idx_filter++) {
> +AVFilterParams *p = ch->filters[idx_filter];
> +
> +if (p->filter_name) {
> +ret = fail_creation_pending(seg, p->filter_name, __func__);
> +goto fail;
> +}
> +
> +if (!p->filter)
> +continue;
> +
> +ret = link_inputs(seg, idx_chain, idx_filter, inputs);
> +if (ret < 0)
> +goto fail;
> +
> +ret = link_outputs(seg, idx_chain, idx_filter, outputs);
> +if (ret < 0)
> +goto fail;
> +}
> +}
> +return 0;
> +fail:
> +avfilter_inout_free(inputs);
> +avfilter_inout_free(outputs);
> +return ret;
> +}
> +
> +int avfilter_graph_segment_apply(AVFilterGraphSegment *seg, int flags,
> + AVFilterInOut **inputs,
> + AVFilterInOut **outputs)
> +{
> +int ret;
> +
> +if (flags)
> +return AVERROR(ENOSYS);
> +
> +ret = avfilter_graph_segment_create_filters(seg, 0);
> +if (ret < 0) {
> +av_log(seg->graph, AV_LOG_ERROR, "Error creating filters\n");
> +return ret;
> +}
> +
> +ret = avfilter_graph_segment_apply_opts(seg, 0);
> +if (ret < 0) {
> +av_log(seg->graph, AV_LOG_ERROR, "Error applying filter options\n");
> +return ret;
> +}
> +
> +ret = avfilter_graph_segment_init(seg, 0);
> +if (ret < 0) {
> +av_log(seg->graph, AV_LOG_ERROR, "Error initializing filters\n");
> +return ret;
> +}
> +
> +ret = avfilter_graph_segment_link(seg, 0, inputs, outputs);
> +if (ret < 0) {
> +av_log(seg->graph, AV_LOG_ERROR, "Error linking filters\n");
> +return ret;
> +}
> +
> +return 0;
> +}
This is all I have found for now, but I still need to look at the tricky
parts of the code more carefully.
--
Nicolas George
___
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".
Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS
Anton Khirnov (12023-01-30): > This hack is used to limit the visibility of some AVFilterLink fields to > only certain files. Replace it with the same pattern that is used e.g. > in lavf AVStream/FFstream and avoid exposing these internal fields in a > public header completely. > --- > libavfilter/avfilter.c | 191 +-- > libavfilter/avfilter.h | 45 - > libavfilter/avfiltergraph.c | 9 +- > libavfilter/buffersink.c | 8 +- > libavfilter/link_internal.h | 69 + > libavfilter/tests/filtfmts.c | 9 +- > 6 files changed, 196 insertions(+), 135 deletions(-) > create mode 100644 libavfilter/link_internal.h This makes the code more verbose, less readable and harder to maintain, so no thanks. If you find a solution that does not require us to remember which field is private and with field is public to prefix them with link-> or li->, it would not have this issue; avoiding this requirement was a prime goal of the current implementation. At least you did not add an indirection like on some other places. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] frame durations for framesync
Paul B Mahol (12023-01-30): > libavfilter support overlapping frames and frames with gaps, look into > aresample filter and its async options. aresample is a very special case. And for audio filters, the duration is carried by nb_samples, not duration. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] frame durations for framesync
Paul B Mahol (12023-01-30): > > The duration of a frame is equal to the difference between its timestamp > > and the timestamp of the next frame. > Only in special cases. No, in libavfilter, always; libavfilter does not support overlapping frames nor gaps. And if you want to make a point, you will have to explain your position with a lot more than four words. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] frame durations for framesync
Paul B Mahol (12023-01-30): > It is not redundant, it describes how much frame lasts. > There is no other way to derive it. The duration of a frame is equal to the difference between its timestamp and the timestamp of the next frame. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH 3/4] lavfi/framesync: add syncing via external timestamp map
Anton Khirnov (12023-01-27): > This is not forcing timestamps on output frames. This is solving the > general problem where the correct matching of input frames is determined > by some external logic. The specific case that is of interest to me is > where this logic is the ffmpeg CLI framerate conversion, which allows > framesync to accurately match videos processed through it. But I can > imagine other cases where this would be useful. You are explaining nothing that was not already present in the commit message, and my interpretation is still: you are engaged in a wrong solution and are trying to make it work, i.e. XY problem <https://en.wikipedia.org/wiki/XY_problem>. Just force the timestamps to the input of framesync filters and you will get what you want on the output. > > And serializing timestamps in decimal in a giant string with newlines in 12 3 4 > > it: definitely no. > Why not? > 'No' with no reasoning and no suggested alternative is not much of an > argument. There were four arguments. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] frame durations for framesync
Paul B Mahol (12023-01-27): > From b4f835c4ef6e0e0bbe6adef8235381e56f3f91df Mon Sep 17 00:00:00 2001 > From: Paul B Mahol > Date: Fri, 27 Jan 2023 23:34:02 +0100 > Subject: [PATCH 1/4] avfilter/framesync: calculate frame duration too > > Signed-off-by: Paul B Mahol > --- > libavfilter/framesync.c | 20 > libavfilter/framesync.h | 15 +++ > 2 files changed, 31 insertions(+), 4 deletions(-) It does not work. For example, with the following filter graph: testsrc2=r=1/3[a];testsrc2=r=1/5[b];[a][b]vstack and adding a line of debug at the beginning of process_frame() in vf_stack.c, you get: got event: pts = 0, duration = 3 got event: pts = 3, duration = 3 ← should be 2 got event: pts = 5, duration = 5 ← should be 1 got event: pts = 6, duration = 3 … got event: pts = 9, duration = 3 got event: pts = 10, duration = 5 got event: pts = 12, duration = 3 got event: pts = 15, duration = 3 got event: pts = 18, duration = 3 got event: pts = 20, duration = 5 I was wrong saying it cannot work, you can compute the duration of the new frame without increasing the latency, but you have to duplicate the whole frame synchronization logic. I was against adding the duration field to libavfilter, and this is a very good illustration of the reason. The duration field is redundant with other information, it takes a lot of code to maintain it, and we cannot trust it anyway. For libavfilter, my advice would be to disregard it entirely. Let us just clear it in buffersrc and be done with it. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] frame durations for framesync
Paul B Mahol (12023-01-27): > From b4f835c4ef6e0e0bbe6adef8235381e56f3f91df Mon Sep 17 00:00:00 2001 > From: Paul B Mahol > Date: Fri, 27 Jan 2023 23:34:02 +0100 > Subject: [PATCH 1/4] avfilter/framesync: calculate frame duration too > > Signed-off-by: Paul B Mahol > --- > libavfilter/framesync.c | 20 > libavfilter/framesync.h | 15 +++ > 2 files changed, 31 insertions(+), 4 deletions(-) Looking into it right now, but I am pretty sure it cannot work. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH 1/5] lavfi/avfilter: export process_options()
Anton Khirnov (12023-01-27): > Send them then. > It's been almost two weeks since my initial RFC, And if you have to wait ten more weeks for a proper review, you will wait ten more weeks. > and the code is quite > straightforward. No, it is not. I have seen likely flaws, but I need time to make sure. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 1/5] lavfi/avfilter: export process_options()
Anton Khirnov (12023-01-27): > If nobody has further comments, I would like to push this set next week. I have already said I would have comments. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 3/4] lavfi/framesync: add syncing via external timestamp map
Anton Khirnov (12023-01-27): > Useful when there is some external process that determines canonical > frame synchronization. E.g. the framerate conversion code in ffmpeg CLI. > --- > doc/filters.texi| 6 ++ > libavfilter/framesync.c | 121 ++-- > libavfilter/framesync.h | 11 > 3 files changed, 132 insertions(+), 6 deletions(-) I agree with Paul, this looks like an instance of a XY problem <https://en.wikipedia.org/wiki/XY_problem>. framesync generates output based on its input. Therefore to force timestamps on output frames you need to force timestamps on input frames. And serializing timestamps in decimal in a giant string with newlines in it: definitely no. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH 2/4] lavfi/framesync: reorder functions to avoid a forward declaration
Anton Khirnov (12023-01-27): > --- > libavfilter/framesync.c | 144 > 1 file changed, 71 insertions(+), 73 deletions(-) Getting rid of the forward declaration would be a good idea. But the order you have put the functions is completely illogical. For example you separated ff_framesync_preinit() and ff_framesync_init() with functions that have nothing to do with initialization. Moving framesync_advance() closer to its call point would probably be a much better idea. So: not like this. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH 1/4] lavfi/framesync: use a local variable to shorten code
Anton Khirnov (12023-01-27):
> ---
> libavfilter/framesync.c | 33 -
> 1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/libavfilter/framesync.c b/libavfilter/framesync.c
> index ee91e4cf68..153db4fa21 100644
> --- a/libavfilter/framesync.c
> +++ b/libavfilter/framesync.c
> @@ -201,24 +201,23 @@ static int framesync_advance(FFFrameSync *fs)
> break;
> }
> for (i = 0; i < fs->nb_in; i++) {
> -if (fs->in[i].pts_next == pts ||
> -(fs->in[i].ts_mode == TS_NEAREST &&
> - fs->in[i].have_next &&
> - fs->in[i].pts_next != INT64_MAX && fs->in[i].pts !=
> AV_NOPTS_VALUE &&
> - fs->in[i].pts_next - pts < pts - fs->in[i].pts) ||
> -(fs->in[i].before == EXT_INFINITY &&
> - fs->in[i].state == STATE_BOF)) {
> -av_frame_free(>in[i].frame);
> -fs->in[i].frame = fs->in[i].frame_next;
> -fs->in[i].pts= fs->in[i].pts_next;
> -fs->in[i].frame_next = NULL;
> -fs->in[i].pts_next = AV_NOPTS_VALUE;
> -fs->in[i].have_next = 0;
> -fs->in[i].state = fs->in[i].frame ? STATE_RUN :
> STATE_EOF;
> -if (fs->in[i].sync == fs->sync_level && fs->in[i].frame)
> +FFFrameSyncIn * const in = >in[i];
Get rid of the const, since the rest of this code does not use anything
similar and the benefit is dubious.
> +
> +if (in->pts_next == pts ||
> +(in->ts_mode == TS_NEAREST && in->have_next &&
> + in->pts_next != INT64_MAX && in->pts != AV_NOPTS_VALUE &&
> + in->pts_next - pts < pts - in->pts) ||
> +(in->before == EXT_INFINITY && in->state == STATE_BOF)) {
> +av_frame_free(>frame);
> +in->frame = in->frame_next;
> +in->pts= in->pts_next;
> +in->frame_next = NULL;
> +in->pts_next = AV_NOPTS_VALUE;
> +in->have_next = 0;
> +in->state = in->frame ? STATE_RUN : STATE_EOF;
> +if (in->sync == fs->sync_level && in->frame)
> fs->frame_ready = 1;
> -if (fs->in[i].state == STATE_EOF &&
> -fs->in[i].after == EXT_STOP)
> +if (in->state == STATE_EOF && in->after == EXT_STOP)
> framesync_eof(fs);
> }
> }
I do not like this change. In fact, the more I think about it the more I
remember that I specifically considered using intermediate pointers for
inputs and deciding against: too much extra noise if done everywhere,
too little benefit for the inconsistency if done only where there are
many.
So: thanks but no.
--
Nicolas George
___
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".
Re: [FFmpeg-devel] [PATCH 4/5] lavfi/graphparser: reimplement avfilter_graph_parse* using new API
Nicolas George (12023-01-20): > - Writing a large patch for a part of the code you know well or for a > new feature is fine, of course. In fact, even for code we know well, we should consult, as long as we are not the only one who knows it well: as long as other developers might also have plans that we would be interfering with, sharing intents early is the only correct thing to do towards our peers. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 4/5] lavfi/graphparser: reimplement avfilter_graph_parse* using new API
Jean-Baptiste Kempf (12023-01-20): > First, maintainer does not mean owner No. But maintainer means person who knows the code very well. > Then, also, in the same email you say "I have no time" and "you should > consult with me before". Those are contradictory. This is not contradictory. I have no time to reply in days, but this patch took more than days in the making, I would have ample chance to take the time. Writing this patch alone, Anton, who has contributed to libavfilter a lot at this time, incurs the risk of neglecting issues that might require rewriting a significant part of it, or even voiding it entirely. Somebody who knows the code and has given a lot of thought to future developments (me for example) could have pointed to those possible issues early, saving a lot of time to Anton. This is not specific to this issue: Why would ANYBODY spend a lot of time and effort writing a far-reaching patch to a part of the code they do not know without consulting their fellow developers who know this part better, the maintainer in particular? I insist, it is a very specific combinations of criteria: - Writing a small patch for a part you do not know well without consulting is fine, because the possible wasted effort is small and the likelihood of severe mistakes is small. It is quadratic. - Writing a large patch for a part of the code you know well or for a new feature is fine, of course. But the combination (1) code that you do not know well, (2) patch that took effort writing, (3) patch that goes far and this is at risk of hard to see issues, then not consulting with people who know the code best seems really absurd. For lack of a better explanation, I perceive cases like that when they happen as a lack of team-spirit that is detrimental to this project. I mean, it would not have taken a lot of time: "Hey guys, I am considering redesigning the graph parsing API in lavfi. Here are my main ideas: ... Do you have comments before I really start?", and it could have made the result a lot better from the get go. Also, it would have been the POLITE thing to do. It would have been the thing to do if one considers me as an equal. -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH 4/5] lavfi/graphparser: reimplement avfilter_graph_parse* using new API
Anton Khirnov (12023-01-20): > --- > libavfilter/graphparser.c | 537 +- > 1 file changed, 126 insertions(+), 411 deletions(-) I notice you removed the "RFC" from the subject line, but I probably have comments. I need some time, my time is very limited these days. By the way, I am baffled that you would write a 30k+ patch on code you do not maintain and do not know well without consulting the maintainer who knows it. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH v4] avformat/dvd: new dvd:// protocol for reading dvd folder/images
Stanislav Ionascu (12023-01-15): > dvd:// protocol uses libdvdread for opening folders and/or disc-images, > it then either identifies the longest title-set, or uses the title-set > parameter. > > After opening the dvd, libdvdread-protocol will read and output all > VOBs, in the cell and pack sequence defined by the program-chain. > > The (split)-VOB content that is streamed from the folder/disc by > libdvdread, is given to ffmpeg probed demuxer/decoder, which decodes > and presents the frames. > > The chapter and language stream information is present in the > side-files (BUP/IFO), which are parsed for identifying the correct > main video-title-set, but not attached to the stream information. Thanks for the new patch. All this information needs to be in the user documentation, with a little more detail on what it means in practice (chapters, languages, late probing, etc.). Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH 1/8] lavfi/vf_scale: allow passing options to swscale directly
Anton Khirnov (12023-01-08): > Avoid using the dictionary mechanism, which is non-introspectable. As > this is the only user of AVFilter.init_dict(), this callback can now be > removed. Series looks good to me, thanks. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH v3] avformat/dvd: new dvd:// protocol for reading dvd folder/images
Stanislav Ionascu (12023-01-08): > dvd:// protocol uses libdvdread for opening folders and/or disc-images, > it then either identifies the longest title-set, or uses the title-set > parameter. > > After opening the dvd, it will read and output all VOBs, in sequence > defined by the program-chain. Thanks for the patch. The reason there was no DVD support in ffmpeg until now is that it is not as straightforward as it seems. Your patch is missing documentation, including documentation about its limitations. Please clarify various points: - VOBs contain not only the movie but also menu data: will it be mixed along with the movie or skipped? - Are the timestamps monotonous over a whole title? - Does ffmpeg correctly detect all the subtitles streams? - Does ffmpeg correctly detect the language of audio and subtitles streams? - Are chapters detected? Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/rtpproto: add support for RTP/UDP socket reuse
Camille Oudot (12023-01-04): > So does it still makes sense to have a patch to pass through a RTP > "reuseaddr" option to the underlying UDP URL "reuse" option? Yes, totally. But let us make it the right way: not just add this option and forward code, but correctly set up the objects so that all options are forwarded as necessary. See libavfilter/framesync.h for an example of code that does this. > Documenting the good use cases of course (although I'd start using it > for the wrong use case ;-)). If yes I'll resubmit the patch. Having several instances of ffmpeg / ffplay / whatever receive the same stream seems like a valid use case. > I'm still planning to have a look on a proper (e.g using only one > shared socket) implementation of rtcp-mux (easy one, I already have a > PoC working) and RTP Bundle (tougher one). Please. I suspect cheating with SO_REUSEADDR would not have been more straightforward anyway: the demuxer for each individual stream would have received the packets for all streams and would have to be taught to distinguish. Regards, -- Nicolas George ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/rtpproto: add support for RTP/UDP socket reuse
Camille Oudot (12023-01-03): > Why does the "reuse" option exists on the "udp" protocol in the first > place? Because it is an useful option. Only not for this use case. Because nobody noticed it was badly named when it was applied. > Why would it not apply to "rtp" also? Because nobody made the UDP socket context a child object of the RTP context. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils: add -mask_url to replace the protocol address in the command with the asterisk (*)
Wujian(Chin) (12023-01-03): > Please review it again, thanks!! You still treat the option differently without a good reason. You still do not protect credentials in options. Not acceptable. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] Would a crypto file be acceptable?
Mark Gaiser (12022-12-28): > > Ok, that made me laugh :) I did not write that. And that feels rude. > Please correct me as I'm probably wrong. > But where did anyone say that there was a working alternative solution that > did not require customizing how ffmpeg is used? > Like disabling safe. Heuristics to not require safe in cases where it is quite safe would be much more acceptable than a redundant ad-hoc format. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] Would a crypto file be acceptable?
Mark Gaiser (12022-12-28): > There's definitely multiple things going on but it's not what you summarize. You forgot the bit where “MU” told you your proposal was mostly redundant, too limited and ad-hoc and would have security implications if more generic, and “DEV” conveniently ignored it. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/rtpproto: add support for RTP/UDP socket reuse
"zhilizhao(赵志立)" (12022-12-26): > Just use the same socket file descriptor. Don’t use OS dependent hack to > implement a feature. SO_REUSEADDR is absolutely not a hack. I agree that it is not the correct way of implementing the feature evoked in this thread, but I insist it is not a hack. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils: add -mask_url to replace the protocol address in the command with the asterisk (*)
* (win32_argc
> + 1);
> if (!win32_argv_utf8) {
> LocalFree(argv_w);
> +get_mask_flag(argc_ptr, argv_ptr);
> return;
> }
>
> @@ -246,6 +318,7 @@ static void prepare_app_arguments(int *argc_ptr, char
> ***argv_ptr)
>
> *argc_ptr = win32_argc;
> *argv_ptr = win32_argv_utf8;
> +get_mask_flag(argc_ptr, argv_ptr);
> }
> #else
> static inline void prepare_app_arguments(int *argc_ptr, char ***argv_ptr)
> @@ -696,10 +769,8 @@ int split_commandline(OptionParseContext *octx, int
> argc, char *argv[],
> {
> int optindex = 1;
> int dashdash = -2;
> -
> /* perform system-dependent conversions for arguments list */
> prepare_app_arguments(, );
> -
> init_parse_context(octx, groups, nb_groups);
> av_log(NULL, AV_LOG_DEBUG, "Splitting the commandline.\n");
>
> diff --git a/fftools/cmdutils.h b/fftools/cmdutils.h
> index 4496221..08c4da7 100644
> --- a/fftools/cmdutils.h
> +++ b/fftools/cmdutils.h
> @@ -50,6 +50,31 @@ extern AVDictionary *format_opts, *codec_opts;
> extern int hide_banner;
>
> /**
> + * Using to mask sensitive info.
> + */
> +void mask_param(int argc, char **argv);
> +
> +/**
> + * Using to copy ori argv.
> + */
> +char **copy_argv(int argc, char **argv);
> +
> +/**
> + * Handle argv and argv_copy.
> + */
> +char **handle_arg_param(int argc, int mask_flag, char **argv);
> +
> +/**
> + * Get mask flag.
> + */
> +int get_mask_flag(int *argc, char ***argv);
> +
> +/**
> + * Free argv.
> + */
> +void free_argv_copy(int argc, char **argv);
> +
> +/**
> * Register a program-specific cleanup routine.
> */
> void register_exit(void (*cb)(int ret));
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 881d6f0..d16eb36 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -3865,9 +3865,9 @@ static int64_t getmaxrss(void)
>
> int main(int argc, char **argv)
> {
> -int ret;
> +int ret, mask_flag;
> BenchmarkTimeStamps ti;
> -
> +char **argv_copy;
> init_dynload();
>
> register_exit(ffmpeg_cleanup);
> @@ -3877,15 +3877,16 @@ int main(int argc, char **argv)
> av_log_set_flags(AV_LOG_SKIP_REPEATED);
> parse_loglevel(argc, argv, options);
>
> +mask_flag = get_mask_flag(, );
> #if CONFIG_AVDEVICE
> avdevice_register_all();
> #endif
> avformat_network_init();
>
> show_banner(argc, argv, options);
> -
> +argv_copy = handle_arg_param(argc, mask_flag, argv);
> /* parse options and open all input/output files */
> -ret = ffmpeg_parse_options(argc, argv);
> +ret = ffmpeg_parse_options(argc, argv_copy);
> if (ret < 0)
> exit_program(1);
>
> @@ -3920,5 +3921,6 @@ int main(int argc, char **argv)
> exit_program(69);
>
> exit_program(received_nb_signals ? 255 : main_return_code);
> +free_argv_copy(argc, argv_copy);
> return main_return_code;
> }
> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> index fc7e1c2..559e417 100644
> --- a/fftools/ffplay.c
> +++ b/fftools/ffplay.c
> @@ -3663,10 +3663,12 @@ void show_help_default(const char *opt, const char
> *arg)
> /* Called from the main */
> int main(int argc, char **argv)
> {
> -int flags;
> +int flags, mask_flag;
> +char **argv_copy;
> VideoState *is;
>
> init_dynload();
> +mask_flag = get_mask_flag(, );
>
> av_log_set_flags(AV_LOG_SKIP_REPEATED);
> parse_loglevel(argc, argv, options);
> @@ -3682,7 +3684,8 @@ int main(int argc, char **argv)
>
> show_banner(argc, argv, options);
>
> -parse_options(NULL, argc, argv, options, opt_input_file);
> +argv_copy = handle_arg_param(argc, mask_flag, argv);
> +parse_options(NULL, argc, argv_copy, options, opt_input_file);
>
> if (!input_filename) {
> show_usage();
> @@ -3759,6 +3762,6 @@ int main(int argc, char **argv)
> event_loop(is);
>
> /* never returns */
> -
> +free_argv_copy(argc, argv_copy);
> return 0;
> }
> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
> index d2f126d..49375bd 100644
> --- a/fftools/ffprobe.c
> +++ b/fftools/ffprobe.c
> @@ -4035,9 +4035,10 @@ int main(int argc, char **argv)
> WriterContext *wctx;
> char *buf;
> char *w_name = NULL, *w_args = NULL;
> -int ret, input_ret, i;
> -
> +int ret, input_ret, i, mask_flag;
> +char **argv_copy;
> init_dynload();
> +mask_flag = get_mask_flag(, );
>
> #if HAVE_THREADS
> ret = pthread_mutex_init(_mutex, NULL);
> @@ -4056,8 +4057,8 @@ int main(int argc, char **argv)
> #endif
>
> show_banner(argc, argv, options);
> -parse_options(NULL, argc, argv, options, opt_input_file);
> -
> +argv_copy = handle_arg_param(argc, mask_flag, argv);
> +parse_options(NULL, argc, argv_copy, options, opt_input_file);
> if (do_show_log)
> av_log_set_callback(log_callback);
>
> @@ -4173,6 +4174,7 @@ end:
> av_freep(_format);
> av_freep(_intervals);
> av_hash_freep();
> +free_argv_copy(argc, argv_copy);
>
> uninit_opts();
> for (i = 0; i < FF_ARRAY_ELEMS(sections); i++)
--
Nicolas George
signature.asc
Description: PGP signature
___
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".
Re: [FFmpeg-devel] Would a crypto file be acceptable?
Tomas Härdin (12022-12-26): > Right. And trying to smuggle in command line options via a file feels > made for exploitation.. This is why my proposal years ago was rejected by Reimar. And this is why concat requires -safe to accept options. To be fair, limiting the case to cryptographic keys would probably not be exploitable, but it is a half measure, too specific to a particular use case. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] Would a crypto file be acceptable?
Tomas Härdin (12022-12-26): > That we want to avoid having keys in the command line is not > unreasonable. A -keyfile argument for crypto: might be appropriate. You are confusing two threads, the issue of credentials visible in the command line is for another proposal. This here is about giving options without giving options, which is a recurring topic. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] 答复: 答复: [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils: add -mask_url to replace the protocol address in the command with the asterisk (*)
Wujian(Chin) (12022-12-24): > This code simplicity does not affect the readability and > maintainability of the program You are not the one who maintain them, that is not your judgement. > The mask effect is like this：ffmpeg -mask_url -i rtsp://tyyy.com --> ffmpeg > -mask_url -i *** I know, it was obvious from the start. > > this way you miss credentials passed through options. > I still don't understand. Can you go into more detail? Look up cryptokey in the documentation for example. > >> >sizeof(*argv2) > Sorry, I didn't get it. This is elementary C and a staple of FFmpeg's coding style. > I understand that mask_url needs to be placed first, And that is an unacceptable misfeature. > because the FFmpeg process of the ps -ef command is viewed as argv, That is nonsense. > Therefore, you need to copy argv to argv_copy immediately, mask the URL in > the argv, > and then argv_copy is a command without mask_url for the following programs. The few milliseconds of the normal options parsing will not make a difference. > I don't understand the difference between the normal option and the mask_url > in use? Your version is a special case that needs to be maintained. That is only acceptable with a very good reason, and you have not provided one. > Looking forward to your reply, thank you. Let me be clear: this here is a review of your code, not a tutorial on becoming a better developer, and even less "what an excellent idea, let us do the work for you". -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] Would a crypto file be acceptable?
Mark Gaiser (12022-12-23): > They actually point to N files. Could also be just 1. Wow, what an interesing and crucial distinction for the discussion. Goodbye. -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] Would a crypto file be acceptable?
Mark Gaiser (12022-12-23): > Then M3U as a format is business logic too. > HLS and MPD are business logic too. > At least, based on your comment, they would fall into that same category. > > The difference between those formats and my suggestion? First difference: they point to several files, yours is about a single file. Second difference: they exist out there, yours you are trying to invent just for that. -- Nicolas George signature.asc Description: PGP signature ___ 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".
