New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment
link

Issue 911823: Refactor FFmpegDemuxerStream codec support checks

Reported by chcunningham@chromium.org, Dec 4 Project Member

Issue description

FFmpegDemuxerStream is only created when an Audio/VideoDecoderConfig is determined to be valid.

For some time this mechanism has been used to prevent creation of decoder streams for codecs that only available when USE_PROPRIETARY_CODECS is defined. Generally this works by only partially converting an AVCodecContext to a *DecoderConfig inside ffmpeg common when the code described is not supported. Audio/VideoDecoderConfig's don't pass IsValid() checks under these partial conversions, causing us to bail on creation of the demuxer stream (as opposed to failing later during decoding).

https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=2526aa69a6bacb7ad8d246e9b6c3ed8cf7b48966&l=260

This is an undocumented and subtle way to enforce that we do not setup streams when we cannot decode them. It is easy to regress by simply converting a little more of the AVCodecContext than we did before (https://bugs.chromium.org/p/chromium/issues/detail?id=784610#c26)

We have a separate mechanism dedicated to checking codec support used by MimeUtil. 

https://cs.chromium.org/chromium/src/media/base/decode_capabilities.h?rcl=8727349f7bd30918e9ed246567fc5fcd94583b6f&l=28

We should use that mechanism here as well to check for codec support before setting up the demuxer stream.
 

Comment 1 by chcunningham@chromium.org, Dec 4

Summary: Refactor FFmpegDemuxerStream codec support checks (was: Refactor FFmpegDemuxerStream creation)

Comment 2 by bugdroid1@chromium.org, Dec 7

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78534522cdb0c3881263661a2aba2606f246bf76

commit 78534522cdb0c3881263661a2aba2606f246bf76
Author: chcunningham <chcunningham@chromium.org>
Date: Fri Dec 07 01:07:27 2018

Refactor decoder support query utils

Until now, media/ decoder capabilities were described by
media/base/decode_capabilities.*. Embedders could override those
abilities via MediaClient. This meant 2 steps to query for
what configs were supported
1) if MediaClient exists, ask it
2) fallback to functions in decode_capabilites

We have two places where these steps should be followed:
MimeUtilInternal and WebMediaCapabilitiesClientImpl. Until now,
only MimeUtilInternal did both.

We anticipate a third caller (when creating FFmpegDemuxerStreams), so
the time is right to put these two steps into a utility function:
	bool IsSupported{Audio|Video}Type(...);

This is defined in a new file: media/base/supported_types.h. media/'s
default capabilities are also described in that file, via
	bool IsDefaultSupported*Type(...);

This change also renames the struct inputs to these APIs from
*Config to *Type. *Config was too close to *DecoderConfig and
*Type is a closer match to the web APIs we're trying to serve
(IsTypeSupported and CanPlayType).

Change-Id: I5c49d28f3d7ad9e824388cb346369795c3a621f9
Bug: 911823
Reviewed-on: https://chromium-review.googlesource.com/c/1361938
Reviewed-by: Sergey Volk <servolk@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614524}
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/chromecast/renderer/cast_content_renderer_client.cc
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/chromecast/renderer/cast_content_renderer_client.h
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/content/renderer/media/render_media_client.cc
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/content/renderer/media/render_media_client.h
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/media/base/BUILD.gn
[delete] https://crrev.com/cc8b1e563f106abfcd3e153bcaf03f3c6a272d81/media/base/decode_capabilities.h
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/media/base/key_systems_unittest.cc
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/media/base/media_client.h
[add] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/media/base/media_types.h
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/media/base/mime_util_internal.cc
[rename] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/media/base/supported_types.cc
[add] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/media/base/supported_types.h
[rename] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/media/base/supported_types_unittest.cc
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/media/blink/webmediacapabilitiesclient_impl.cc
[modify] https://crrev.com/78534522cdb0c3881263661a2aba2606f246bf76/media/renderers/audio_renderer_impl_unittest.cc

Comment 3 by bugdroid1@chromium.org, Dec 8

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9cb3a21fbe1dce467f9a2107df8dfe2c3d820420

commit 9cb3a21fbe1dce467f9a2107df8dfe2c3d820420
Author: chcunningham <chcunningham@chromium.org>
Date: Sat Dec 08 00:36:07 2018

Refactor mime util proprietary codec checks.

Mime util is slowly getting out of the business of saying what codecs
are supported. Most of this is extracted to media/base/supported_types.h
where it can be easily shared between mime_util, MediaCapabilities and
future callers. This common utility should condition its answers based on
proprietary_codecs build flags and mime util should just defer to it.

Bug: 911823
Change-Id: I5ff7d64caf2bcadfb9b5d281bee8e243b2df6970
Reviewed-on: https://chromium-review.googlesource.com/c/1362217
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614887}
[modify] https://crrev.com/9cb3a21fbe1dce467f9a2107df8dfe2c3d820420/media/base/media_types.h
[modify] https://crrev.com/9cb3a21fbe1dce467f9a2107df8dfe2c3d820420/media/base/mime_util_internal.cc
[modify] https://crrev.com/9cb3a21fbe1dce467f9a2107df8dfe2c3d820420/media/base/mime_util_internal.h
[modify] https://crrev.com/9cb3a21fbe1dce467f9a2107df8dfe2c3d820420/media/base/supported_types.cc
[modify] https://crrev.com/9cb3a21fbe1dce467f9a2107df8dfe2c3d820420/media/base/supported_types_unittest.cc

Sign in to add a comment