media::DecoderBuffer needs some clean up |
||||
Issue descriptionSome random ideas I had when I work on the DecoderBuffer code recently: - We allow a 0-byte DecoderBuffer, which is confusing. The meaning of different |data| and |size| combinations could be confusing, and they are not well documented. Here's the current state: * Null data, size must be zero: EOS buffer * Non-null data, zero size: 0-byte buffer * Non-null data, non-zero size: normal buffer. It's unclear what the use case of 0-byte buffer is. From tests (ChunkDemuxerTest.SeekCompletesWithoutTextCues), it seems we do generate 0-byte buffers, where the |side_data| isn't empty. I am not familiar with this code. If this is a legit use case, we should document it clearly. If not, we should remove the support of 0-byte buffer to avoid further confusion. - There are a lot of duplicate code between DecoderBuffer and DataBuffer. It would be great if we could combine them, e.g. have a common base class.
,
Nov 8 2016
Also, it seems a bit hacky to use data==nullptr as the indicator for a EOS buffer. In the example in #1, the buffer size is zero, but we still have a non-null data pointer. Transporting this buffer over mojo is tricky as well, since we don't have any data to send, but we still need to make sure the received DecoderBuffer has a non-null data. I suggest we have a dedicated flag (e.g. is_end_of_stream) to signal whether the buffer is EOS.
,
Nov 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/98da7afb903cd39d1c695b33c5fa23e59d9da0e8 commit 98da7afb903cd39d1c695b33c5fa23e59d9da0e8 Author: xhwang <xhwang@chromium.org> Date: Wed Nov 09 07:18:17 2016 media: Support transporting 0-byte DecoderBuffer over mojo BUG=663438 TEST=Added a new unitttest. Review-Url: https://codereview.chromium.org/2484763005 Cr-Commit-Position: refs/heads/master@{#430883} [modify] https://crrev.com/98da7afb903cd39d1c695b33c5fa23e59d9da0e8/media/mojo/common/media_type_converters.cc [modify] https://crrev.com/98da7afb903cd39d1c695b33c5fa23e59d9da0e8/media/mojo/common/mojo_decoder_buffer_converter.cc [modify] https://crrev.com/98da7afb903cd39d1c695b33c5fa23e59d9da0e8/media/mojo/common/mojo_decoder_buffer_converter_unittest.cc [modify] https://crrev.com/98da7afb903cd39d1c695b33c5fa23e59d9da0e8/media/mojo/interfaces/media_types.mojom
,
Nov 9 2016
Also, |side_data| doesn't have any documentation. It seems to be used by vpx_video_decoder [1]. I think we have multiple places when we convert/transport DecoderBuffer, we ignored the |side_data| [2, 3]. We should clarify the use case of |side_data|, check whether it's really necessary. If so, document it, and make sure the |side_data| is always preserved during conversion/decryption/transmission. [1] https://cs.chromium.org/chromium/src/media/filters/vpx_video_decoder.cc?rcl=0&l=667 [2] https://cs.chromium.org/chromium/src/media/filters/decrypting_demuxer_stream.cc?rcl=0&l=241 [3] https://cs.chromium.org/chromium/src/media/filters/gpu_video_decoder.cc?rcl=0&l=435
,
Nov 9 2016
FWIW, this is the CL that added |side_data|: https://codereview.chromium.org/12263013
,
Feb 17 2017
,
May 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3287cb2391a584ab3b10c4a1099290fb419cd797 commit 3287cb2391a584ab3b10c4a1099290fb419cd797 Author: jrummell <jrummell@chromium.org> Date: Tue May 23 22:11:25 2017 Have FFmpeg Decoders handle 0 byte buffers instead of FFmpeg Starting in M60 avcodec_decode_video2() fails if an empty buffer is passed in. So don't have FFmpegAudioDecoder or FFmpegVideoDecoder call FFmpeg routines if there is no data to process. BUG= 718142 ,663438 TEST=affected test still passes Review-Url: https://codereview.chromium.org/2885643002 Cr-Commit-Position: refs/heads/master@{#474080} [modify] https://crrev.com/3287cb2391a584ab3b10c4a1099290fb419cd797/media/filters/ffmpeg_audio_decoder.cc [modify] https://crrev.com/3287cb2391a584ab3b10c4a1099290fb419cd797/media/filters/ffmpeg_video_decoder.cc [modify] https://crrev.com/3287cb2391a584ab3b10c4a1099290fb419cd797/media/filters/ffmpeg_video_decoder_unittest.cc
,
Mar 21 2018
,
Mar 21 2018
,
Mar 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/491a2c2c94dbed6c4d65164918ba7f1d6be0a44d commit 491a2c2c94dbed6c4d65164918ba7f1d6be0a44d Author: Matt Wolenetz <wolenetz@chromium.org> Date: Tue Mar 27 17:59:39 2018 MSE: Drop empty AV buffers emitted by Stream Parser Since FFmpeg decode of 0-byte audio and video buffers is now disallowed, and FFmpegDemuxer skips enqueuing such packets into its DemuxerStream, this change does similar for MSE: Any 0-byte |data| audio or video buffers are dropped during initial processing in FrameProcessor::ProcessFrames(). No 0-byte |data| text buffers are dropped, because: * We don't use FFmpeg to decode in-band MSE text, and * In-band MSE text buffers' |side_data| may contain meaningful information. (This assumption isn't asserted by this change). Note, this change takes the simpler route of dropping 0-byte AV buffers before they're buffered by the coded frame processing algorithm. A more complex alternative to skip them when reading from SourceBufferStream was rejected in hopes this simpler change is sufficient. Includes ability in ChunkDemuxerTest to vary |block_size_| instead of always using |kBlockSize|. While the product change is closer to FrameProcessor, FrameProcessorTests always encode the coded frame's timestamp as the coded frame contents to enable precise verification of processing results, so the test of this change is done via ChunkDemuxerTest instead. BUG= 823375 ,663438 TEST=ChunkDemuxerTest.ZeroLengthFramesDropped Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ie6a802ba64a576f0219ca7c400fc7dec3a45fbf6 Reviewed-on: https://chromium-review.googlesource.com/974427 Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Cr-Commit-Position: refs/heads/master@{#546159} [modify] https://crrev.com/491a2c2c94dbed6c4d65164918ba7f1d6be0a44d/media/base/test_helpers.h [modify] https://crrev.com/491a2c2c94dbed6c4d65164918ba7f1d6be0a44d/media/filters/chunk_demuxer_unittest.cc [modify] https://crrev.com/491a2c2c94dbed6c4d65164918ba7f1d6be0a44d/media/filters/frame_processor.cc [modify] https://crrev.com/491a2c2c94dbed6c4d65164918ba7f1d6be0a44d/media/filters/frame_processor.h [modify] https://crrev.com/491a2c2c94dbed6c4d65164918ba7f1d6be0a44d/media/formats/webm/cluster_builder.cc |
||||
►
Sign in to add a comment |
||||
Comment 1 Deleted