This seems to be a needed fix for adding the accelerated 10bit decoding which I'm currently looking into. Anyone working on this? If not, I'll have a look.

+tmathmeyer since this might affect his work on issue 888873. We have vp9+h264 parsers that we might be able to use on the first packet to determine the true profile if there's not another option.

I haven't started work here and wont have time in the next week. sreerenj, happy to have help - lets chat about the approach. 

Our flow is generally to avoid forking ffmpeg, so any change there should go first to upstream and then get cherry picked or grabbed the next time we do an ffmpeg roll in chrome (roughly once per milestone).

I think step 1 is to start reading the profile attribute from the avcodeccontext
https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavcodec/avcodec.h?rcl=458e9fd3f8e8c913a739389c65dfaf1f77ee9106&l=2858

And then map the profile to its media::VideoCodecProfile equivalent
https://cs.chromium.org/chromium/src/media/base/video_codecs.h?q=media::VideoCodecProfile&sq=package:chromium&dr=Ss&l=50

Similar to what we're doing already for the codec attribute:
https://cs.chromium.org/chromium/src/media/ffmpeg/ffmpeg_common.cc?rcl=22a175cbcd669cd3d64f2b76f6c1f7d7b2527fc1&l=176


But that still won't give you the vp9 nor av1 profiles - ffmpeg is not currently parsing the WebM CodecPrivate in the matroska demuxer
(currently being ignored to avoid having it pop up in ffmpeg's extradata)
http://ffmpeg.org/pipermail/ffmpeg-devel/2018-February/225548.html

We don't need it to be in extradata either, but we should parse it in a manner similar to below and set the results in the AVCodecContext
https://cs.chromium.org/chromium/src/media/formats/webm/webm_video_client.cc?rcl=9d90876be428603f0e129d4d58804efec78e9f6c&l=66

Note that this is best effort. Not all files will have the CodecPrivate for us to parse. 

Similar work may be need for the mp4 demuxer - I'm less familiar though.

Looking at the ffmpeg code, we should easily be able to enable the vp8, vp9, and av1 parsers for our ffmpeg build. We can try enabling the h264 one too, but it looks like it might pull in a bunch of code...

Here's a binary size try job for Android which will tell us the total size increase for all parsers:
https://chromium-review.googlesource.com/c/chromium/src/+/1278964

Locally that results in a 100k increase with component builds, but dead-code linking maybe smaller. :O

It's as I expected:
https://storage.googleapis.com/chrome-supersize/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchromium-binary-size-trybot-results%2Fandroid-binary-size%2F2018%2F10%2F12%2F75210.ndjson&diff_mode=on

vp8, vp9 parsers == ~0.46kb, h264 parser == ~62.3kb mostly because it pulls in sei decoding and dsp. av1 parser doesn't exist in our current roll, but should show up in the next one that Chris is doing.

I propose we enable the vp8, vp9, av1 ones, but keep the h264 one disabled and find a way to use our built in H264Parser instead to collect the information if we need it. If we're lucky it's just in the extradata....

> I propose we enable the vp8, vp9, av1 ones...

Sounds good. I'll double check that enabling the vp9/av1 parsers gives the profile info. If yes, I'll enable those in the next ffmpeg roll. 

> but keep the h264 one disabled and find a way to use our built in H264Parser instead to collect the information if we need it. If we're lucky it's just in the extradata...

sreerenj.balachandran@intel.com - can you take this part? It looks like the avcc is being writtne to the extradata[0]. You can make a new AVCDecoderConfigurationRecord and call its Parse() method, passing in the extradata. Then the profile info can be extracted and converted like we do here[1].


[0] https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavformat/mov.c?rcl=458e9fd3f8e8c913a739389c65dfaf1f77ee9106&l=1911

[1] https://cs.chromium.org/chromium/src/media/formats/mp4/box_definitions.cc?rcl=ed6d37e317890fe8fbe33499151b64a694b9e1f6&l=833

@Dale, @chcunningham
Thanks a lot for the suggestions 	 

@Dale,
When do you expect the next ffmpeg roll? So I guess the plan is that you will take care of the profile setting for all codecs except h264. right?

I have some code to enable VA-API based 10-bit decoding which is mostly involves changes to add a new 10 bit raw video format support. Currently I'm testing it with some hacks to hard-code the VP9 profile.
Wondering whether I should create a new bug report for this feature now or wait for a fix for this profile-setting-issue (784610) ?  

>sreerenj.balachandran@intel.com - can you take this part? It looks like the >avcc....

Sure, I can have a look. Hope it is not an immediate requirement(something needed by tomorrow) since I'm new to the chromium code base, but I have a decent knowledge in the codec stuffs :)

> When do you expect the next ffmpeg roll? 

I'll start it next week (10/22). Should be done a few days after that.

> So I guess the plan is that you will take care of the profile setting for all codecs except h264. right?

Right. We can't ship the H264 parser on android due to the size.

> I have some code to enable VA-API based 10-bit decoding...

I defer to Dale.

> Hope it is not an immediate requirement(something needed by tomorrow)

Nope. P2/P3. It will be at least a week before my part is done.

You'll need to chat with posciak@ and hiroh@ about any VAAPI changes.

@dale. Sure, vaapi 10 bit decode is already implemented, only thing missing is profile and format negotiation. profile negotiation should be handled in this bug and format handling here: https://bugs.chromium.org/p/chromium/issues/detail?id=896010

Here is the h264 part: https://chromium-review.googlesource.com/c/chromium/src/+/1292605

Please let me know if anything is wrong!

https://chromium-review.googlesource.com/c/chromium/src/+/1292605 generally looks good. One comment on the review. 

Little update on my part...

VP9 parser knows the profile but doesn't (yet) ouptut to codec context. Will fix.
https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavcodec/vp9_parser.c?rcl=458e9fd3f8e8c913a739389c65dfaf1f77ee9106&l=42

AV1 parser just landed upstream and sets the profile :)
https://github.com/FFmpeg/FFmpeg/commit/ebc3d04b8df4e11b1343090fed1014832d5cf46d

Any update?

Discussion for ffmpeg vp9 parser ongoing. Will ping soon if I don't hear back. 
http://ffmpeg.org/pipermail/ffmpeg-devel/2018-November/235961.html

For your CL, waiting to hear Dale's opinion on the ifdef part.

Thank you! I have updated h264 patch in my CL too.
BTW, it would be great if some one can add my name to author's list since it causing pre-submit warnings.
Sreerenj Balachandran<sreerenj.balachandran@intel.com>

You should do that in your cl I think.

Done. Thank you Dale.

Just curious about the work flow. IICU, now I should wait for either Dale or chris to give a +1 .right? Am I supposed to do the "CQ DRY RUN"?

> I should wait for either Dale or chris to give a +1 .right

Yep. Dale will +1 when all comments are addressed (few new ones added this morning).

> Am I supposed to do the "CQ DRY RUN"?

You can do a dry run at any time to see if your change breaks anything unexpected. The real CQ run will happen once you've got approval and actually submit the change to be merged. The change will only merge if CQ passes, so dry runs are just a way to catch that earlier. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ffa5c05c6e68b8abfbe4f23f63292ae35d7772b6

commit ffa5c05c6e68b8abfbe4f23f63292ae35d7772b6
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date: Tue Nov 13 22:29:52 2018

media: Add H264 profile extraction support

Without the ffmpeg decoder configured, libavformat is unable to get the
codec profile and chromium ffmpeg wrapper code choose some defaults.
This will mess up the decode scenarios where non-ffmpeg decoders like vaapi
are in use. Fortunately, we can still get hold of the ffmpeg's
AVStream extradata param which should contain the
H264 AVCDecoderConfigurationRecord (ISO/IEC 14496-15).
So we use internal parser for extracting profile information
from this AVCDecoderConfigurationRecord.

BUG=784610
TEST=media_unittests:FFmpegCommonTest.VerifyH264Profile

Change-Id: Id67c7026eefb5bb73e6c5fbbab7a362cb4553ab1
Reviewed-on: https://chromium-review.googlesource.com/c/1292605
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607769}
[modify] https://crrev.com/ffa5c05c6e68b8abfbe4f23f63292ae35d7772b6/media/ffmpeg/BUILD.gn
[modify] https://crrev.com/ffa5c05c6e68b8abfbe4f23f63292ae35d7772b6/media/ffmpeg/ffmpeg_common.cc
[modify] https://crrev.com/ffa5c05c6e68b8abfbe4f23f63292ae35d7772b6/media/ffmpeg/ffmpeg_common_unittest.cc
[modify] https://crrev.com/ffa5c05c6e68b8abfbe4f23f63292ae35d7772b6/media/formats/BUILD.gn

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f57bfc33cce988ef3cdd57a5960ec86705260a6

commit 1f57bfc33cce988ef3cdd57a5960ec86705260a6
Author: Chrome Cunningham <chcunningham@chromium.org>
Date: Tue Nov 13 23:49:26 2018

Revert "media: Add H264 profile extraction support"

This reverts commit ffa5c05c6e68b8abfbe4f23f63292ae35d7772b6.

Reason for revert: broke webkit_tests on Mojo Linux bot. Likely something to do with gn config that lacks proprietary codecs.

Original change's description:
> media: Add H264 profile extraction support
> 
> Without the ffmpeg decoder configured, libavformat is unable to get the
> codec profile and chromium ffmpeg wrapper code choose some defaults.
> This will mess up the decode scenarios where non-ffmpeg decoders like vaapi
> are in use. Fortunately, we can still get hold of the ffmpeg's
> AVStream extradata param which should contain the
> H264 AVCDecoderConfigurationRecord (ISO/IEC 14496-15).
> So we use internal parser for extracting profile information
> from this AVCDecoderConfigurationRecord.
> 
> BUG=784610
> TEST=media_unittests:FFmpegCommonTest.VerifyH264Profile
> 
> Change-Id: Id67c7026eefb5bb73e6c5fbbab7a362cb4553ab1
> Reviewed-on: https://chromium-review.googlesource.com/c/1292605
> Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
> Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#607769}

TBR=dalecurtis@chromium.org,chcunningham@chromium.org,sreerenj.balachandran@intel.com

Change-Id: I1cc9a3236d7187115566dff882c586b00dca713b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 784610
Reviewed-on: https://chromium-review.googlesource.com/c/1334664
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607817}
[modify] https://crrev.com/1f57bfc33cce988ef3cdd57a5960ec86705260a6/media/ffmpeg/BUILD.gn
[modify] https://crrev.com/1f57bfc33cce988ef3cdd57a5960ec86705260a6/media/ffmpeg/ffmpeg_common.cc
[modify] https://crrev.com/1f57bfc33cce988ef3cdd57a5960ec86705260a6/media/ffmpeg/ffmpeg_common_unittest.cc
[modify] https://crrev.com/1f57bfc33cce988ef3cdd57a5960ec86705260a6/media/formats/BUILD.gn

Hm, I don't to where to put the fix for this. @Chris will you look into the issue? 
Were can I see the build errors? Wondering why it was not caught by the tryjob?

Looking into it now. The CQ has a bot that ran this test, but the bot has a slightly different config from the one that later failed. :(

Fix in progress here https://chromium-review.googlesource.com/c/chromium/src/+/1338869

I forgot to include a link to the failed build - apologies.
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mojo%20Linux/22134

These layout tests failed
* media/sources-fallback-codecs.html
* media/video-source-media.html
* virtual/video-surface-layer/media/sources-fallback-codecs.html
* virtual/video-surface-layer/media/video-source-media.html

These tests run through a video/audio with nested <source> tags and verify that source fallback logic works. The MojoLinux build doesn't have proprietary codecs turned on so we expect .mp4 to fall back to .ogv. The break happened because we started setting enough parts (codec, pixel layout, profile) of the h264 VideoDecoderConfig for it be considered valid, which meant we didn't skip over it and tried to setup the decoder (which failed). 

The tests pass if you simply ifdef out the whole h264 section when USE_PROPRIETARY_CODECS isn't set, causing VideoDecoderConfig to be invalid and avoiding the creation of that demuxer stream. 

This config-validity-contract is totally undocumented!

Also, I think we should probably have similar tests for proprietary audio codecs that would fail with todays code... still looking into it. 

@chris Thank you for sharing the build failure log and also for the fix.