Reorganize bitstream converter classes.

Reorganize media bitstream converter classes.

- Remove IdentityBitstreamConverter which is obsolete.
- Remove FFmpegBitstreamConverter as it's not really used. Currently GpuVideoDecoder is the only place where EnableBitstreamConverter is called, and EnableBitstreamConverter is the only caller of FFmpegBitstreamConverter, but only when decoding mpeg4, whereas GVD only works with h264 ATM.
- Remove BitstreamConverter since after removal of FFmpegBitstreamConverter, we only have one implementation of BitstreamConverter.
- Remove BitstreamConverterTest.
- Rename FFmpegH264BitstreamConverter to FFmpegH264ToAnnexBBitstreamConverter.
- Rename H264BitstreamConverter to H264ToAnnexBBitstreamConverter.
- Move H264ToAnnexBBitstreamConverter from media/base to media/filters.

BUG=none
TEST=media_unittests

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

[xhwang, 2012-07-11 17:20:27]

Remove IdentityBitstreamConverter as discussed offline. Please review.

[Ami GONE FROM CHROMIUM, 2012-07-11 17:25:47]

http://codereview.chromium.org/10690140/diff/1/media/filters/bitstream_conver...
File media/filters/bitstream_converter.h (right):

http://codereview.chromium.org/10690140/diff/1/media/filters/bitstream_conver...
media/filters/bitstream_converter.h:46: class MEDIA_EXPORT
FFmpegBitstreamConverter : public BitstreamConverter {
Any reason not to collapse this into the base now that it's the only impl?

[Ami GONE FROM CHROMIUM, 2012-07-11 17:43:25]

Oops, forgot about FFmpegH264BitstreamConverter.  LGTM

[xhwang, 2012-07-11 18:05:50]

On 2012/07/11 17:43:25, Ami Fischman wrote:
> Oops, forgot about FFmpegH264BitstreamConverter.  LGTM

Thanks for the review. Some more thoughts on this:

1, BitstreamConverter should be named FFmpegBitstreamConverter, since AVPacket
is FFmpeg specific.
2, The current FFmpegBitstreamConverter should be renamed to
FFmpegFilterBitstreamConverter because it uses bitstream filter. Then it's more
obvious that FFmpegFilterBitstreamConverter is a sibling of
FFmpegH264BitstreamConverter.
3, Move h264_bitstream_converter* from media/base to media/filter
4, Not sure why we pack two classes in the current bitstream_converter.*. The
BitstreamConverterTest actually tests the FFmpegBitstreamConverter. Should we
spearate them into two files?

[Ami GONE FROM CHROMIUM, 2012-07-11 18:27:09]

I had similar thoughts looking through this stuff.


> 1, BitstreamConverter should be named FFmpegBitstreamConverter, since
> AVPacket is FFmpeg specific.
>

Yep.


> 2, The current FFmpegBitstreamConverter should be renamed
> to FFmpegFilterBitstreamConverter because it uses bitstream filter. Then
> it's more obvious that FFmpegFilterBitstreamConverter is a sibling
> of FFmpegH264BitstreamConverter.
>

I don't think Filter vs. H264 is clear enough.
BUT, does anyone even *use* the Filter one??
AFAICT GpuVideoDecoder is the only place where EnableBitstreamConverter is
called, and EnableBitstreamConverter is the only caller
of FFmpegBitstreamConverter, but only when decoding mpeg4, whereas GVD only
works with h264 ATM.  So I think this is all dead code and you can delete
BitstreamConverter and FFmpegBitstreamConverter and teach
FFmpegDemuxerStream about FFmpegH264BitstreamConverter (which should be
renamed FFmpegH264ToAnnexBBitstreamConverter) directly.

3, Move h264_bitstream_converter* from media/base to media/filter
>

SGTM.


> 4, Not sure why we pack two classes in the current bitstream_converter.*.
> The
> BitstreamConverterTest actually tests the FFmpegBitstreamConverter. Should
> we
> spearate them into two files?


No, we should delete this fileset, preserving any test bits that apply to
the annexb converter.

-a

[xhwang, 2012-07-11 21:11:10]

On 2012/07/11 18:27:09, Ami Fischman wrote:
> I had similar thoughts looking through this stuff.
> 
> 
> > 1, BitstreamConverter should be named FFmpegBitstreamConverter, since
> > AVPacket is FFmpeg specific.
> >
> 
> Yep.

BitstreamConverter/FFmpegBitstreamConverter removed.
> 
> 
> > 2, The current FFmpegBitstreamConverter should be renamed
> > to FFmpegFilterBitstreamConverter because it uses bitstream filter. Then
> > it's more obvious that FFmpegFilterBitstreamConverter is a sibling
> > of FFmpegH264BitstreamConverter.
> >
> 
> I don't think Filter vs. H264 is clear enough.
> BUT, does anyone even *use* the Filter one??
> AFAICT GpuVideoDecoder is the only place where EnableBitstreamConverter is
> called, and EnableBitstreamConverter is the only caller
> of FFmpegBitstreamConverter, but only when decoding mpeg4, whereas GVD only
> works with h264 ATM.  So I think this is all dead code and you can delete
> BitstreamConverter and FFmpegBitstreamConverter and teach
> FFmpegDemuxerStream about FFmpegH264BitstreamConverter (which should be
> renamed FFmpegH264ToAnnexBBitstreamConverter) directly.
> 
> 3, Move h264_bitstream_converter* from media/base to media/filter
> >
> 
> SGTM.

Done.
> 
> 
> > 4, Not sure why we pack two classes in the current bitstream_converter.*.
> > The
> > BitstreamConverterTest actually tests the FFmpegBitstreamConverter. Should
> > we
> > spearate them into two files?
> 
> 
> No, we should delete this fileset, preserving any test bits that apply to
> the annexb converter.

BitstreamConverterTest removed.
> 
> -a

Hello fischman,

Mostly removing/renaming. Please review again. There's no functionality change.
Please take a closer look at the comment on
FFmpegH264ToAnnexBBitstreamConverter::ConvertPacket().

xhwang

[Ami GONE FROM CHROMIUM, 2012-07-11 22:33:17]

LGTM % nits
Please test the videotestmatrix just in case.

http://codereview.chromium.org/10690140/diff/3002/media/filters/bitstream_con...
File media/filters/bitstream_converter.h (left):

http://codereview.chromium.org/10690140/diff/3002/media/filters/bitstream_con...
media/filters/bitstream_converter.h:3: // found in the LICENSE file.
rietveld is showing this as a modification, not a delete.  Can you make sure
it's deleted?  
(ditto for .cc file)

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:213: if (stream_->codec->codec_id ==
CODEC_ID_H264) {
this would be both simpler and more informative at failure as a:
DCHECK_EQ(stream_->codec->codec_id, CODEC_ID_H264);
and then drop the if/else/NOTREACHED.

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.h:128: // Used to translate bitstream formats.
comment is silly

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_h264_t...
File media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.h (right):

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_h264_t...
media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.h:17: // Bit stream
converter that converts H.264 bit stream based FFmpeg packets into
s/Bit stream/Bitstream/
here and elsewhere

http://codereview.chromium.org/10690140/diff/3002/media/filters/h264_to_annex...
File media/filters/h264_to_annex_b_bitstream_converter.cc (right):

http://codereview.chromium.org/10690140/diff/3002/media/filters/h264_to_annex...
media/filters/h264_to_annex_b_bitstream_converter.cc:151:
ConvertAVCDecoderConfigurationRecordToByteStream(
style-guide is silent on how this should be indented, but this particular choice
is poor IMO b/c it lines up with the next line.
Rename to AVCDecoderConfigToByteStream()?
(I don't know what "ByteStream" means in this context...)

[xhwang, 2012-07-12 00:21:45]

Will test the videotestmatrix before committing.

http://codereview.chromium.org/10690140/diff/3002/media/filters/bitstream_con...
File media/filters/bitstream_converter.h (left):

http://codereview.chromium.org/10690140/diff/3002/media/filters/bitstream_con...
media/filters/bitstream_converter.h:3: // found in the LICENSE file.
On 2012/07/11 22:33:17, Ami Fischman wrote:
> rietveld is showing this as a modification, not a delete.  Can you make sure
> it's deleted?  
> (ditto for .cc file)

I don't know why it's shown as modified :\, it's not in my local folder and git
shows it's deleted:

diff --git a/media/filters/bitstream_converter.cc
b/media/filters/bitstream_converter.cc
deleted file mode 100644
index 8708f69..0000000
--- a/media/filters/bitstream_converter.cc
+++ /dev/null


diff --git a/media/filters/bitstream_converter.h
b/media/filters/bitstream_converter.h
deleted file mode 100644
index c08143e..0000000
--- a/media/filters/bitstream_converter.h
+++ /dev/null

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:213: if (stream_->codec->codec_id ==
CODEC_ID_H264) {
On 2012/07/11 22:33:17, Ami Fischman wrote:
> this would be both simpler and more informative at failure as a:
> DCHECK_EQ(stream_->codec->codec_id, CODEC_ID_H264);
> and then drop the if/else/NOTREACHED.

Done. So I assume only H.264 would trigger the GpuVideoDecoder and this code
path?

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.h:128: // Used to translate bitstream formats.
On 2012/07/11 22:33:17, Ami Fischman wrote:
> comment is silly

Done.

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_h264_t...
File media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.h (right):

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_h264_t...
media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.h:17: // Bit stream
converter that converts H.264 bit stream based FFmpeg packets into
On 2012/07/11 22:33:17, Ami Fischman wrote:
> s/Bit stream/Bitstream/
> here and elsewhere

Done.

http://codereview.chromium.org/10690140/diff/3002/media/filters/h264_to_annex...
File media/filters/h264_to_annex_b_bitstream_converter.cc (right):

http://codereview.chromium.org/10690140/diff/3002/media/filters/h264_to_annex...
media/filters/h264_to_annex_b_bitstream_converter.cc:151:
ConvertAVCDecoderConfigurationRecordToByteStream(
On 2012/07/11 22:33:17, Ami Fischman wrote:
> style-guide is silent on how this should be indented, but this particular
choice
> is poor IMO b/c it lines up with the next line.
> Rename to AVCDecoderConfigToByteStream()?
> (I don't know what "ByteStream" means in this context...)

Done.

[Ami GONE FROM CHROMIUM, 2012-07-12 00:27:54]

still LGTM

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/10690140/diff/3002/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:213: if (stream_->codec->codec_id ==
CODEC_ID_H264) {
On 2012/07/12 00:21:45, xhwang wrote:
> On 2012/07/11 22:33:17, Ami Fischman wrote:
> > this would be both simpler and more informative at failure as a:
> > DCHECK_EQ(stream_->codec->codec_id, CODEC_ID_H264);
> > and then drop the if/else/NOTREACHED.
> 
> Done. So I assume only H.264 would trigger the GpuVideoDecoder and this code
> path?

Yep

[commit-bot: I haz the power, 2012-07-12 00:40:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10690140/3004

[commit-bot: I haz the power, 2012-07-12 02:07:42]

Failed to apply patch for media/media.gyp:
While running patch -p1 --forward --force;
patching file media/media.gyp
Hunk #2 succeeded at 218 (offset 4 lines).
Hunk #3 succeeded at 227 (offset 4 lines).
Hunk #4 succeeded at 315 (offset 4 lines).
Hunk #5 succeeded at 322 (offset 4 lines).
Hunk #6 FAILED at 677.
Hunk #7 succeeded at 698 (offset 6 lines).
Hunk #8 succeeded at 740 (offset 6 lines).
1 out of 8 hunks FAILED -- saving rejects to file media/media.gyp.rej

[commit-bot: I haz the power, 2012-07-12 15:25:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10690140/2021

[commit-bot: I haz the power, 2012-07-12 15:25:45]

Failed to apply patch for media/media.gyp:
While running patch -p1 --forward --force;
patching file media/media.gyp
Hunk #2 succeeded at 218 (offset 4 lines).
Hunk #3 succeeded at 227 (offset 4 lines).
Hunk #4 succeeded at 315 (offset 4 lines).
Hunk #5 succeeded at 322 (offset 4 lines).
Hunk #6 FAILED at 677.
Hunk #7 succeeded at 698 (offset 6 lines).
Hunk #8 succeeded at 740 (offset 6 lines).
1 out of 8 hunks FAILED -- saving rejects to file media/media.gyp.rej

[commit-bot: I haz the power, 2012-07-12 16:45:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10690140/5006

[commit-bot: I haz the power, 2012-07-12 17:51:29]

Try job failure for 10690140-5006 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-12 18:13:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10690140/5006

[commit-bot: I haz the power, 2012-07-12 20:00:25]

Change committed as 146420