On 5/5/2017 2:26 AM, wm4 wrote:
On Fri, 5 May 2017 01:11:17 -0700
Aaron Levinson <alevi...@aracnet.com> wrote:
As I said on IRC, I'm skeptic against this, but I'm also not sure
whether I understand the situation.
First, your change seems a bit fragile, and almost relies on
coincidences. This could easily break in the future. (And if we add a
FATE test, it would probably hit random people trying to change
ffmpeg.c, and would cause them more work.)
I guess I don't understand why you think this change is fragile, relies
on coincidences, and could easily break in the future.
Well, the implemented and working logic is that ffmpeg.c waits until it
has encoded packets for every stream, and then writes the file headers.
That should work pretty well to let metadata naturally "flow" through
decoder, filters, and encoder. But your case doesn't fit right into it,
so you apparently have to make it somehow call do_video_out() more.
(Also a thing I don't understand - do_video_out() should have been
called at least once to encode a video packet.)
That's not how ffmpeg.c behaves today. It writes the file headers
before it has any encoded packets for the _last_ output stream that is
initialized. With my change, I make sure that the file headers aren't
written until after it has had an opportunity to generate encoded
packets for at least one input frame for all output streams, although
there are some FATE test cases in which input frames aren't ready yet by
this point, which explains why the existence of the fallback in my patch
is necessary.
It doesn't call do_video_out() any more times with my patch than prior
to my patch. It just changes the order of when check_init_output_file()
is called with respect to do_video_out() (and do_audio_out()).
And looking at how field_order is set, the existing code doesn't make
sense in the first place.
So maybe it would be better to fix the actual problem, instead of
adding another workaround.
At least that's my thinking.
The existing
code is already fragile.
Indeed. That (and me being sleep deprived) probably contributes to that
I just don't understand your fix.
Also feel free to ignore me - I'm neither maintainer of ffmpeg.c nor an
authority on it.
As I indicated in previous e-mails on this
topic, the behavior of the current code can change depending on timing
when there is more than one stream. This patch should make things more
deterministic. And, I do think it is appropriate to add an interlaced
video decoding FATE test, although there is no point to doing so until
this bug is fixed, as the current behavior is broken.
Looking at the current do_video_out() function (which btw. is way too
long and also has broken indentation), I see the following line:
mux_par->field_order = AV_FIELD_PROGRESSIVE;
(and similar assignments to field_order to set interlaced modes)
This is apparently run on every frame. This looks pretty wrong to me.
It should _never_ change the codecpar after headers were written. This
is simply invalid API use.
If this is really a parameter that can change per frame, and that the
_muxer_ needs this information (per packet I suppose), a side data
signaling the field order should be added. mpegtsenc.c doesn't seem to
read field_order at all, though.
So I remain confused.
Yes, I agree that the approach of changing field_order for every frame
is not the right way to go, but this code was already existing. A
future patch could undertake the task of handling interlaced video in a
more proper way, and I wrote a TODO comment to that point.
I don't understand that comment either. As I already said,
do_video_out() has to be _necessarily_ called before the muxer can be
initialized (aka writing headers).
The way the code is currently structured, do_video_out() is called by
reap_filters() after it calls init_output_stream(), and it is
init_output_stream() that makes the call to check_init_output_file(),
which is where the call to avformat_write_header() is done.
reap_filters() is the only function that calls do_video_out(), and I
think it is pretty clear that this will be done after it calls
init_output_stream() from looking at the source code.
My TODO has to do with setting up the field order based on header
information, rather than waiting till it gets the first input frame.
That's the theory at least, but I don't currently know how to do that in
practice within ffmpeg.c.
Aaron Levinson
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
