Fix av sync for webm files with altref video frames. Altref frames...

Fix av sync for webm files with altref video frames. Altref frames
need to be decoded but they do not produce a valid output frame. The
pts_heap_ was not getting popped for altref frames.

BUG=51014
TEST=Play content form sync test and make sure the content plays
back in sync.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55596

[fbarchard, 2010-08-03 17:35:38]

LGTM
As you say it has to do with altref frames
1. I confirmed that encoding without altref frames fixes sync video
2. The problem should affect youtube, but
a) only some videos make use of altref
b) many videos have content where you can't tell if audio is out of sync with
video.
c) unless humans watch and listen, av sync issues can be subtle and easy to
miss.

[scherkus (not reviewing), 2010-08-03 18:08:23]

Hold on -- this code path technically affects all formats

are we sure there are no regressions?

[fgalligan, 2010-08-03 19:03:38]

I put the pop in wrong place. Let me fix that.

On Tue, Aug 3, 2010 at 2:08 PM, <scherkus@chromium.org> wrote:

> Hold on -- this code path technically affects all formats
>
> are we sure there are no regressions?
>
>
> http://codereview.chromium.org/3086009/show
>

[fgalligan1, 2010-08-03 19:27:33]

Moved the pop tot the proper place. I ran through the media layout tests and all
passed with patch set 2.

[scherkus (not reviewing), 2010-08-03 19:37:08]

ah this looks like it makes more sense.. however media_unittests are failing :)

You should see if you can make
FFmpegVideoDecoderTest.DoDecode_TestStateTransition simluate the WebM altref
frame

[fgalligan, 2010-08-04 16:01:29]

Sorry I had to go through and read the gtest and gmock documentation.

I should be able to simulate an altref frame in the test. I just want to
test the state transitions and pts handling with some mp4 files first.

Frank

On Tue, Aug 3, 2010 at 3:37 PM, <scherkus@chromium.org> wrote:

> ah this looks like it makes more sense.. however media_unittests are
> failing :)
>
> You should see if you can make
> FFmpegVideoDecoderTest.DoDecode_TestStateTransition simluate the WebM
> altref
> frame
>
>
> http://codereview.chromium.org/3086009/show
>

[fgalligan1, 2010-08-04 23:26:30]

I changed the behavior so we do not pop the pts_heap anymore. This change should
not affect h.264 or theora because both codecs render directly into memory
returned by get_buffer(). get_buffer() initializes avframe->reordered_opaque
with the avcodec->reordered_opaque. The old CL could cause the first frame in an
h.264 to have a timestamp that was off by a few frames.


I still want to change the behavior of the pts_heap as it is broken right now. I
don't know if we want to change that before M7. 

Also later on we can add support for reordered_opaque to VP8 but we need to
change codec sdk to render into memory provided from outside the sdk.

[fgalligan1, 2010-08-09 20:17:53]

Looks like there is some precedence for doing it this way already.

http://mailman.videolan.org/pipermail/vlc-commits/2010-May/001451.html

[Alpha Left Google, 2010-08-10 01:45:01]

LGTM given this doesn't break h264 and theora.

[scherkus (not reviewing), 2010-08-17 01:45:17]

ahh nice find!

so does this make vp8/webm use reordered_opaque as expected and not the pts
heap?

I have a dream of hopefully one day removing the pts heap codepath completely as
its very error prone

also does this obselete issue 50457 or should we expect a fix inside
FFmpeg/libvpx to properly use reordered_opaque?

[fgalligan1, 2010-08-17 13:14:49]

On 2010/08/17 01:45:17, scherkus wrote:
> ahh nice find!
> 
> so does this make vp8/webm use reordered_opaque as expected and not the pts
> heap?
> 
> I have a dream of hopefully one day removing the pts heap codepath completely
as
> its very error prone
> 
> also does this obselete issue 50457 or should we expect a fix inside
> FFmpeg/libvpx to properly use reordered_opaque?

Yes this should make it so vp8 will only use the pts_heap for a frame that has
an actual pts of 0, just like the other codecs. 

Over the weekend I thinking of a better solution for handling pts_heap and
altref, but if vp8 is the only codec that chrome supports that used to pass back
0 in rendered_opaque than a better solution would be to get rid of the pts_heap
and treat 0 pts as 0 and not av_no_pts.

Issue 50457 is still open. We should wait until we have proper support in libvpx
for direct rendering before we change the code in FFmpeg.