VP9 Fixes

[dwbuiten]

Not sure it's teeeeechnically good to handle the avcodec_receieve_frame stuff the way I have.

Also, this doesn't bother to handle incorrectly muxed VP9 MKV files because... well, I can't be arsed.

See #319.

CC: @wm4

[ghost]

Have you filed a bug report?

[dwbuiten]

I still cannot discern what the actual intention of the API is (if this should work or not). Is it a bug? Will I get told off for not calling avodec_receive_frame until I hit AVERROR(EAGAIN) after every packet? I'll file a bug with a small C program to reproduce if it is worth the time.

... But mostly I don't want to deal with Carl and his lack of understanding of anything related to an API and not a CLI tool.

[ghost]

I'm not sure what exactly you're trying to do here (not familiar with the code), but I think the doxygen on the decode API is pretty clear. Anything that doesn't match with it is a bug. I know mediacodec had issues (later fixed AFAIK), so these things happen.

How is this new condition even hit, though? It checks for Ret != 0 above, so in the else branch Ret == 0 should always hold true.

Yeah, I wasn't seriously suggesting to report it to the ffmpeg bug tracker.

[dwbuiten]

How is this new condition even hit, though? It checks for Ret != 0 above, so in the else branch Ret == 0 should always hold true.

Woops, yeah. It was a last minute change before I pushed, I originally only had it check for codec==VP9. Will fix.

I'll make a test program + file.

[dwbuiten]

@wm4 here is a repro program: https://gist.github.com/dwbuiten/595e1e0ea494ec702afb47973b891ff5

And sample to match: https://www.dropbox.com/s/my8yb9k25wg7qae/test.webm?dl=0

[dwbuiten]

I see the following

Yes and directly below it, it says it isn't explicitly required:

Using the API as outlined above is highly recommended. But it is also possible to call functions outside of this rigid schema.

Misleading / poor doc at best if it is required.

For the sample you provided it returns 0 two times (and if you check the actual frame data returned, it's different in both), then EAGAIN the third time, for at least five different packets within the 500 iterations of the loop you wrote.

How exactly can one VP9 superframe contain two visible frames? I thought it was always one invisible and one visible at most, per VP9 packet. And this seems to be the case when threads==1.

This behavior changes with threads. With threads == 1, I don't get the corrupt-looking output and i get what seems to be correct number of frames out, from the same code. Is this intended? The sample code provided is hardcoded to 16 threads to show this. Is it possible this is related to thread delay?

But it does. In your repro case you're not checking the return value of avcodec_send_packet. If you do, you'll notice it will return EAGAIN in some cases because the decoder/bsf were not done returning the remaining frames from the previous packet and hadn't yet gotten around to fetch the packet you had previously pushed.

You say it does, but then describe how it doesn't (since it ditches any data from frames it hasn't returned), no?

I admit i may be making a wrong analysis (wm4 is after all the author of this API), but what i know is that ffmpeg and ffplay all handle this sample right by calling receive_frame repeatedly until it returns EAGAIN, and making your repro program do the same also fixes the issue for it.

I don't think this is entirely correct, since threads are involved here, and the behavior is different (and what I would expect it to work like) when threads==1.

Further, I don't think we can generically wrap in a do{}while(ret != AVERROR(EAGAIN)) since other codecs (e.g. H.264 PAFF, etc.) rely on accurately calculating frame/field reorder delay along with thread delay for frame accurate seeking (the aforementioned packet<->frame index).

[jamrial]

Yes and directly below it, it says it isn't explicitly required:
Misleading / poor doc at best if it is required.

Not needing to follow the API usage example explicitly means you can do something else than call receive_frame in a loop, like for example call send_packet in a loop, then receive_frame or whatever, as you quoted earlier. But what you should not do is ignore the return value of either function, because they tell you if things went as expected or not.

How exactly can one VP9 superframe contain two visible frames? I thought it was always one invisible and one visible at most, per VP9 packet. And this seems to be the case when threads==1.

I don't think this sample has more than one visible frame per superframe, but avcodec_receive_frame does effectively return two different frames in two consecutive calls in at least five cases within the 500 iterations in your repro program, as both your "fix" and a do {} while(!EAGAIN) loop reveal.
What you say about threading however does hint that something may be wrong somewhere in libavcodec. The two consecutive frames may not be after all generated from the same packet data, and some delay may be at play instead, because with threads == 1 i see that your repro program otherwise unmodified always returns 0 for both send_packet and receive_frame.
The generic code in decode.c handling auto inserted bsfs may not be playing nice with bsfs that effectively return more than one output packet per input packet (as it's the case with vp9_superframe_split).

You say it does, but then describe how it doesn't (since it ditches any data from frames it hasn't returned), no?

I gave you a detailed example of how send_packet will return EAGAIN if previous packet data (buffered internally by generic code) was not pulled internally by a receive_frame call.
If you ignore that error, discard the rejected packet, and try to send another, regardless of that one being accepted or not, the dropped one is lost and decoding is ruined. This is what's happening in the repro program when threads > 1.
Try adding a bunch of printf to see what both functions return on each iteration with or without threading.

So regardless of there being a bug in libavcodec or not, you should at least check for EAGAIN in avcodec_send_packet. Consider what wm4 wrote in the relevant avcodec_send_packet call in ffmpeg.c

// In particular, we don't expect AVERROR(EAGAIN), because we read all
// decoded frames with avcodec_receive_frame() until done.

The latter being something you say you don't want to do.

[dwbuiten]

Not needing to follow the API usage example explicitly means you can do something else than call receive_frame in a loop, like for example call send_packet in a loop, then receive_frame or whatever, as you quoted earlier. But what you should not do is ignore the return value of either function, because they tell you if things went as expected or not.

Certaintly not how I read it at all, and not how several others I've asked bout this have read it. I argue that the very fact that we're arguing over how to interpret the document meants it is poor / not sufficient.

What you say about threading however does hint that something may be wrong somewhere in libavcodec. The two consecutive frames may not be after all generated from the same packet data, and some delay may be at play instead, because with threads == 1 i see that your repro program otherwise unmodified always returns 0 for both send_packet and receive_frame.
The generic code in decode.c handling auto inserted bsfs may not be playing nice with bsfs that effectively return more than one output packet per input packet (as it's the case with vp9_superframe_split).

Curious how this one pans out.

So regardless of there being a bug in libavcodec or not, you should at least check for EAGAIN in avcodec_send_packet.

I'll add this.

Consider what wm4 wrote in the relevant avcodec_send_packet call in ffmpeg.c

I should not have to read ffmpeg.c to gain information on a public API. However, noted.

[jamrial]

Yeah, frame threading code delaying frames it seems. The first ten or so packets are consumed before the first frame is ever output. Try setting slice threading in your repro program, also with 16 threads. Output is fine then.

Not sure if bug in any case, seeing that actually looking at the return values of the decode API and feeding packets/pulling frames as required works around this.

[dwbuiten]

Yeah, frame threading code delaying frames it seems. The first ten or so packets are consumed before the first frame is ever output. Try setting slice threading in your repro program, also with 16 threads. Output is fine then.

Thread delay has always been known, but it is usually constant/calculable as avctx->thread_count -1, but this apparently does not hold for VP9? This is kind of a pain for trying to implement generic (non-codec/format specific) frame accurate indexed seeking.

Further to that, thread delay shouldn't cause any more delay after the initial N packets, but that seems to not be the case here; that is, we see non-0 returns midstream we wouldn't normally see (and don't see with threads==1). To my knowledge, this is not correct behavior.

[dwbuiten]

I'll have to get around to redoing/fixing this, since it is now needed to support AV1, too.