On 02/03/14 18:25, Derek Buitenhuis wrote:
> 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.
I can change it and update the documentation accordingly.
>> +@chapter Modes
>> +NUT has some variants signaled by using the flags field in its main header.
>
> I'm not sure "variants" is correct word...
If you want to suggest a better name I'm all hears.
>> +@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?
In order to mix them, you have 1 initial syncpoint that would be
timestamped if you mix pipe+network. But the assumption of pipe mode is
that you just want something as low overhead as possible in order that
would be consumed immediately.
The network mode is something that was only in the "spec" and nobody
implemented before. I can just split this patch in 3:
- standard compliance flag for formats (used also for muxing hevc)
- nut pipe (the only one I really care about)
- nut network (just to match the specification somehow)
>> + int strict_std_compliance;
>> } AVFormatContext;
>
> Should be a separate patch.
Can be done.
> What's the significance of having a min version?
Sanity checking.
> Inline defines... meh.
I can make an enum if you like.
> s/2/NUT_MIN_VERSION/
Right.
> De we care about sanity checking?
We just print it.
>> + if (nut->flags & NUT_PIPE) {
>> + return AVERROR(ENOSYS);
>> + }
>
> Extra braces.
Ok.
>> - 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.
Hopefully we would merge all the experimental changes and agree on
stamping version 4. Those changes are in my tree since years and I
mentioned them on the nut ml.
>> + 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.
I'll add something more verbose and clear.
>> + 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).
Makes sense indeed.
>
>> //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?
Makes sense having a macro.
>
>> + 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?
nut stream -> nus you write the timestamp in timebase units, the
timebase is per-stream.
> Also, extra braces.
It is more than a line and I had something more involving later.
> Overall, I like the idea. I pipe nut myself.
Thanks a lot for your feedback.
lu
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
