ffmpeg reset bitstream converters on Seek

ffmpeg reset bitstream converters on Seek

We should not assume that the underlying decoders
keep their state after a Seek.
So for example, the SPS/PPS should be added again.

BUG=140371
Committed: https://crrev.com/562c4aca3eb509c8c9913a80ef3e156b2a8f3a41
Cr-Commit-Position: refs/heads/master@{#302154}

[kjoswiak, 2014-10-27 20:21:10]

The CQ bit was checked by kjoswiak@chromium.org

[kjoswiak, 2014-10-27 20:21:11]

The CQ bit was unchecked by kjoswiak@chromium.org

[kjoswiak, 2014-10-27 20:24:09]

From Chromecast repo, fix for http://crbug.com/140371

[DaleCurtis, 2014-10-27 20:36:02]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org, xhwang@chromium.org

[DaleCurtis, 2014-10-27 20:36:02]

looks good, but +xhwang in case he has any feelings about the location.

Do you plan to clean up the rest of the VDA code per the linked bug?

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:438: // If a bitstream converter exists, reset
it.
DCHECK unnecessary since you access it directly below.

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:445: if (bitstream_converter_ != NULL) {
if (!bitstream_converter_)
  return;

bitstream_converter_.reset(new ...).

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:615: if (video) {
No {} necessary.

[kjoswiak, 2014-10-27 20:37:54]

kjoswiak@chromium.org changed reviewers:
+ posciak@chromium.org, scherkus@chromium.org
- xhwang@chromium.org

[kjoswiak, 2014-10-27 20:38:20]

kjoswiak@chromium.org changed reviewers:
+ xingnan.wang@chromium.org

[kjoswiak, 2014-10-27 20:42:08]

On 2014/10/27 20:36:02, DaleCurtis wrote:
> looks good, but +xhwang in case he has any feelings about the location.
> 
> Do you plan to clean up the rest of the VDA code per the linked bug?
> 
>
https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer.cc
> File media/filters/ffmpeg_demuxer.cc (right):
> 
>
https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
> media/filters/ffmpeg_demuxer.cc:438: // If a bitstream converter exists, reset
> it.
> DCHECK unnecessary since you access it directly below.
> 
>
https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
> media/filters/ffmpeg_demuxer.cc:445: if (bitstream_converter_ != NULL) {
> if (!bitstream_converter_)
>   return;
> 
> bitstream_converter_.reset(new ...).
> 
>
https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
> media/filters/ffmpeg_demuxer.cc:615: if (video) {
> No {} necessary.

Wasn't planning to do clean up atm, mainly just helping upstreaming some fixes
from chromecast

also @posciak, was told to run this by you as this might be relevant to chrome
os hardware decoding

[kjoswiak, 2014-10-27 20:45:15]

kjoswiak@chromium.org changed reviewers:
+ xhwang@chromium.org
- xingnan.wang@chromium.org

[kjoswiak, 2014-10-27 20:47:32]

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:438: // If a bitstream converter exists, reset
it.
On 2014/10/27 20:36:02, DaleCurtis wrote:
> DCHECK unnecessary since you access it directly below.

Done.

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:445: if (bitstream_converter_ != NULL) {
On 2014/10/27 20:36:02, DaleCurtis wrote:
> if (!bitstream_converter_)
>   return;
> 
> bitstream_converter_.reset(new ...).

Done.

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:615: if (video) {
On 2014/10/27 20:36:02, DaleCurtis wrote:
> No {} necessary.

Done.

[xhwang, 2014-10-27 20:59:21]

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:445: if (bitstream_converter_ != NULL) {
How about do the same as l.152?

 if (stream_->codec->codec_id == AV_CODEC_ID_H264) {
    bitstream_converter_.reset(
        new FFmpegH264ToAnnexBBitstreamConverter(stream_->codec));

The code about bitstream_converter_enabled_ and bitstream_converter_ is a bit
convoluted. We always create the converted if codec is H264, but we only use it
when EnableBitstreamConverter() is called. I think we should do some cleanup
here...

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:612: // H264 and VP8 require that we resend the
header after flush.
The bit stream converter is really only for H264.

[kjoswiak, 2014-10-27 21:34:17]

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:445: if (bitstream_converter_ != NULL) {
On 2014/10/27 20:59:21, xhwang wrote:
> How about do the same as l.152?
> 
>  if (stream_->codec->codec_id == AV_CODEC_ID_H264) {
>     bitstream_converter_.reset(
>         new FFmpegH264ToAnnexBBitstreamConverter(stream_->codec));
> 
> The code about bitstream_converter_enabled_ and bitstream_converter_ is a bit
> convoluted. We always create the converted if codec is H264, but we only use
it
> when EnableBitstreamConverter() is called. I think we should do some cleanup
> here...

Ya, it does seem a little weird. Should I just go ahead and try to move creation
into EnableBitstreamConverter?

[kjoswiak, 2014-10-27 21:56:41]

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:445: if (bitstream_converter_ != NULL) {
On 2014/10/27 21:34:17, kjoswiak wrote:
> On 2014/10/27 20:59:21, xhwang wrote:
> > How about do the same as l.152?
> > 
> >  if (stream_->codec->codec_id == AV_CODEC_ID_H264) {
> >     bitstream_converter_.reset(
> >         new FFmpegH264ToAnnexBBitstreamConverter(stream_->codec));
> > 
> > The code about bitstream_converter_enabled_ and bitstream_converter_ is a
bit
> > convoluted. We always create the converted if codec is H264, but we only use
> it
> > when EnableBitstreamConverter() is called. I think we should do some cleanup
> > here...
> 
> Ya, it does seem a little weird. Should I just go ahead and try to move
creation
> into EnableBitstreamConverter?

https://chromiumcodereview.appspot.com/10879057 is why, going to look into to
see if still necessary

[kjoswiak, 2014-10-27 23:49:59]

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:445: if (bitstream_converter_ != NULL) {
On 2014/10/27 21:56:41, kjoswiak wrote:
> On 2014/10/27 21:34:17, kjoswiak wrote:
> > On 2014/10/27 20:59:21, xhwang wrote:
> > > How about do the same as l.152?
> > > 
> > >  if (stream_->codec->codec_id == AV_CODEC_ID_H264) {
> > >     bitstream_converter_.reset(
> > >         new FFmpegH264ToAnnexBBitstreamConverter(stream_->codec));
> > > 
> > > The code about bitstream_converter_enabled_ and bitstream_converter_ is a
> bit
> > > convoluted. We always create the converted if codec is H264, but we only
use
> > it
> > > when EnableBitstreamConverter() is called. I think we should do some
cleanup
> > > here...
> > 
> > Ya, it does seem a little weird. Should I just go ahead and try to move
> creation
> > into EnableBitstreamConverter?
> 
> https://chromiumcodereview.appspot.com/10879057 is why, going to look into to
> see if still necessary

Primary issue is that we weren't guaranteed that stream_->codec still exists,
think its safer to just leave as is

https://codereview.chromium.org/683573002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:612: // H264 and VP8 require that we resend the
header after flush.
On 2014/10/27 20:59:21, xhwang wrote:
> The bit stream converter is really only for H264.

Done.

[kjoswiak, 2014-10-29 20:24:27]

So is CL fine, or should I aim to restructure some things? I also have a aac
bitstream converter used by chromecast and potentially an abstraction of
converter interface I put together that I would like to upstream

Also, thoughts on attached comment to line 447?

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:447: new
FFmpegH264ToAnnexBBitstreamConverter(stream_->codec));
Hmm, given https://code.google.com/p/chromium/issues/detail?id=144432 /
https://chromiumcodereview.appspot.com/10879057 worried this could cause same
issue

[xhwang, 2014-10-30 03:36:20]

Sorry for the delay. Let's land this CL then see how we can reconstruct things.

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:388: last_packet_duration_ = kNoTimestamp();
ditto, call ResetBitstreamConverter() here?

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:438: // If a bitstream converter exists, reset
it.
This comment doesn't belong here.

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:439: if (!bitstream_converter_enabled_)
bitstream_converter_enabled_ is inited in ctor, then it's always in #if
defined(USE_PROPRIETARY_CODECS). Can we also put it after l.442? Then you can
combine this check with the check on l.443.

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:615: video->ResetBitstreamConverter();
Instead of doing this, can we handle ResetbitstreamConverter() in
FFmpegDemuxerStream::FlushBuffers()? FlushBuffers() will always be called after
the seek is done. See l.1089.

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.h:117: void ResetBitstreamConverter();
See comment below. We can call this in FlushBuffers (see l.73). In that case, we
can make this function private.

[kjoswiak, 2014-10-30 17:43:06]

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:438: // If a bitstream converter exists, reset
it.
On 2014/10/30 03:36:20, xhwang wrote:
> This comment doesn't belong here.

Done.

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:439: if (!bitstream_converter_enabled_)
On 2014/10/30 03:36:20, xhwang wrote:
> bitstream_converter_enabled_ is inited in ctor, then it's always in #if
> defined(USE_PROPRIETARY_CODECS). Can we also put it after l.442? Then you can
> combine this check with the check on l.443.

Done.

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.h:117: void ResetBitstreamConverter();
On 2014/10/30 03:36:20, xhwang wrote:
> See comment below. We can call this in FlushBuffers (see l.73). In that case,
we
> can make this function private.

Done.

[kjoswiak, 2014-10-30 17:44:33]

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/683573002/diff/60001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:615: video->ResetBitstreamConverter();
On 2014/10/30 03:36:20, xhwang wrote:
> Instead of doing this, can we handle ResetbitstreamConverter() in
> FFmpegDemuxerStream::FlushBuffers()? FlushBuffers() will always be called
after
> the seek is done. See l.1089.

Done.

[xhwang, 2014-10-30 17:50:07]

Thanks! LGTM

[kjoswiak, 2014-10-30 18:19:07]

On 2014/10/30 17:50:07, xhwang wrote:
> Thanks! LGTM

Alright, anyone else have any comments before I commit? Might like at least an
ACK from posciak, if this issue is relevant to you

[DaleCurtis, 2014-10-30 18:23:59]

lgtm

[kjoswiak, 2014-10-30 19:46:51]

The CQ bit was checked by kjoswiak@chromium.org

[commit-bot: I haz the power, 2014-10-30 19:48:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/683573002/80001

[commit-bot: I haz the power, 2014-10-30 21:09:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-30 21:10:32]

Patchset 5 (id:??) landed as
https://crrev.com/562c4aca3eb509c8c9913a80ef3e156b2a8f3a41
Cr-Commit-Position: refs/heads/master@{#302154}