On 3/1/2014 11:06 PM, Luca Barbato wrote:
> ---
>
> Now it does not write the optional flags if the version isn't 4.
>
> doc/muxers.texi | 16 ++++++++++++++++
> doc/nut.texi | 21 +++++++++++++++++++++
> libavformat/avformat.h | 6 ++++++
> libavformat/nut.h | 9 ++++++++-
> libavformat/nutdec.c | 28 +++++++++++++++++++++++-----
> libavformat/nutenc.c | 42 +++++++++++++++++++++++++++++++++++++++---
> libavformat/options_table.h | 5 +++++
> libavformat/version.h | 2 +-
> 8 files changed, 119 insertions(+), 10 deletions(-)
[...]
> +@section nut
> +
> +@table @option
> +@item -nut_flags @var{flags}
> +Modify the nut muxing behaviour in specific ways:
> +@table @option
> +@item @var{pipe} disables the syncpoints, reducing the overhead but making
> the stream non-seekable;
> +@item @var{broadcast} extends the syncpoint with a wallclock field.
> +@end table
> +None of the flags are supposed to be used for storage purposes.
> +@end table
> +
> +@example
> +avconv -i INPUT -f_strict experimental -nut_flags pipe - | processor
> +@end example
Is there any reason you can't split these into two options. The whole
"flags"-style
options remind me a lot of the bad old days of the FFmpeg cli (-sws_flags
anyone?),
and how much I loathed them.
> +@chapter Modes
> +NUT has some variants signaled by using the flags field in its main header.
I'm not sure "variants" is correct word...
> +
> +@multitable @columnfractions .4 .4
> +@item BROADCAST @tab Extend the syncpoint to report the sender wallclock
> +@item PIPE @tab Omit completely the syncpoint
> +@end multitable
This seems kinda of odd. Wouldn't broadcasts want to stream like pipe mode, but
with
wallclock info?
> + /**
> + * Allow non-standard and experimental extension
> + * @see AVCodecContext.strict_std_compliance
> + */
> + int strict_std_compliance;
> } AVFormatContext;
Should be a separate patch.
> -#define NUT_VERSION 3
> +#define NUT_MAX_VERSION 4
> +#define NUT_STABLE_VERSION 3
> +#define NUT_MIN_VERSION 2
What's the significance of having a min version?
> +#define NUT_BROADCAST 1 // use extended syncpoints
> +#define NUT_PIPE 2 // do not write syncpoints
> + int flags;
> + int version; // version currently in use
> } NUTContext;
Inline defines... meh.
> - tmp = ffio_read_varlen(bc);
> - if (tmp < 2 && tmp > NUT_VERSION) {
> + nut->version = ffio_read_varlen(bc);
> + if (tmp < 2 && tmp > NUT_MAX_VERSION) {
s/2/NUT_MIN_VERSION/
> av_log(s, AV_LOG_ERROR, "Version %"PRId64" not supported.\n",
> tmp);
> return AVERROR(ENOSYS);
> @@ -320,6 +320,10 @@ static int decode_main_header(NUTContext *nut)
> assert(nut->header_len[0] == 0);
> }
>
> + if (nut->version > 3) {
> + nut->flags = ffio_read_varlen(bc);
> + }
Extra braces.
s/3/NUT_STABLE_VERSION/
> + if (nut->flags & NUT_BROADCAST) {
> + tmp = ffio_read_varlen(bc);
De we care about sanity checking?
> + if (nut->flags & NUT_PIPE) {
> + return AVERROR(ENOSYS);
> + }
Extra braces.
> - ff_put_v(bc, NUT_VERSION);
> + ff_put_v(bc, nut->version);
It occurs to me, Michael sent an RFC for nut a while back, so they may have,
or intend to also bump the version, which will made two differing version 4
variants... dear god please no.
> + if (nut->version > 3)
> + ff_put_v(bc, nut->flags);
s/3/NUT_STABLE_VERSION/
> + nut->version = NUT_STABLE_VERSION + !!nut->flags;
> + if (nut->flags && s->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL)
> {
> + av_log(s, AV_LOG_ERROR,
> + "Version %d requested, please set -strict experimental in "
> + "order to enable it.\n",
> + nut->version);
Confusing. "version %d" doesn't mean much to users -- print what features they
tried to enable instead.
> + return AVERROR_EXPERIMENTAL;
Using this return code will produce a 2nd message which says (all of) nut is
experimental, which is very confusing to the user. I recommend AVERROR(ENOSYS).
> //FIXME: Ensure store_sp is 1 in the first place.
Unrelated but lulz.
> - if (store_sp) {
> + if (store_sp &&
> + (!(nut->flags & NUT_PIPE) || nut->last_syncpoint_pos == INT_MIN)) {
Super-tiny-nit: !(nut->flags & NUT_PIPE) seems to be used a lot -- perhaps it
could do with a macro or var?
> + if (nut->flags & NUT_BROADCAST) {
> + put_tt(nut, nus->time_base, dyn_bc,
> + av_rescale_q(av_gettime(), AV_TIME_BASE_Q,
> *nus->time_base));
> + }
Typo? nus?
Also, extra braces.
[...]
Overall, I like the idea. I pipe nut myself.
- Derek
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
