similar to bug 1111328 We currently rely on the demuxer data to determine the display dimension of the video. With Annex B and in order to properly support bug 1128381, we can't rely on the demuxer data as those values may be dynamic

Extract display size from SPS, read aspect ration. The vui decoder was actually never called :(. Remove all the fields we will never use and complete documentation.

use display size extracted from SPS. This really drops the need for the VideoConfig parameter

Use display size extracted from SPS. This adds proper display ratio support to FFmpeg decoder.

Wrap FFmpeg in the AVCC wrapper. FFmpeg only ever use the first SPS found. It's up to the consumer to handle change of video size etc.. Seeing that we don't only use the AVCC one and let the AVCC Wrapper handle it for us.

Comment on attachment 8573164 [details] [diff] [review] Part1 - Extract display dimension from SPS NAL Review of attachment 8573164 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: media/libstagefright/binding/H264.cpp @@ +77,5 @@ > chroma_format_idc = 1; > + video_format = 5; > + colour_primaries = 2; > + transfer_characteristics = 2; > + sample_ratio = 1.0; Where do all these default values come from? @@ +131,5 @@ > +static int32_t > +ConditionDimension(float aValue) > +{ > + // This will exclude NaNs and too-big values. > + if (aValue > 1.0 && aValue <= INT32_MAX) Would be nice to use MAX_VIDEO_* from VideoUtils here, but I guess that breaks the abstraction barriers. I think INT32_MAX is safe enough. @@ +250,5 @@ > + // Determine display size. > + if (aDest.sample_ratio > 1.0) { > + // Increase the intrinsic width > + aDest.display_width = > + ConditionDimension(aDest.sample_ratio * aDest.pic_width); pic_width first, please, to match the else clause. @@ +273,2 @@ > > + switch (aDest.aspect_ratio_idc) { Spec reference for this, please. E.g. // ITU T-REC-H.264 table E-1. @@ +371,5 @@ > + // 160:99 > + /* > + 540x576 16:9 frame with horizontal overscan > + */ > + aDest.sample_ratio = 16.0 / 11.0; Cut and paste error? This should be 160. / 99. ? Comment doesn't match the spec either. What about 14-16? @@ +376,5 @@ > + break; > + case 255: > + aDest.sar_width = aBr.ReadBits(16); > + aDest.sar_height = aBr.ReadBits(16); > + aDest.sample_ratio = float(aDest.sar_width) / float(aDest.sar_height); Please document here that you're relying on NaN handling downstream. Or better yet, just don't update sample_ratio if sar_width or sar_height are zero so no one has to worry about it. @@ -272,5 @@ > aDest.fixed_frame_rate_flag = aBr.ReadBit(); > } > - > - bool hrd_present = false; > - if (aBr.ReadBit()) { // nal_hrd_parameters_present_flag Do we no longer need to consume these bits to advance aBr's state?

Comment on attachment 8573166 [details] [diff] [review] Part2. Use display dimension from SPS NAL Review of attachment 8573166 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. This will need some testing on wild content though. I don't think it's a great candidate for getting mac dormant support in 37. :/ ::: dom/media/fmp4/apple/AppleVDADecoder.cpp @@ +57,5 @@ > spsdata.pic_width && spsdata.pic_height) { > mPictureWidth = spsdata.pic_width; > mPictureHeight = spsdata.pic_height; > + mDisplayWidth = spsdata.display_width; > + mDisplayHeight = spsdata.display_height; So this is a major change in behaviour, right? This will unconditionally set the sample aspect ratio to 1.0 if the SPS has no vui table, or if it doesn't specify a ratio, overriding whatever's in the file header. How many wild files have that kind of mismatch? @@ +63,5 @@ > // we need to queue that many frames in order to guarantee proper > // pts frames ordering. Use a minimum of 4 to ensure proper playback of > // non compliant videos. > mMaxRefFrames = > + std::min(std::max(mMaxRefFrames, spsdata.max_num_ref_frames + 1), 16u); Does this belong here? I don't see what it has to do with the rest of the patch. @@ +267,5 @@ > info.mHasVideo = true; > gfx::IntRect visible = gfx::IntRect(0, > 0, > + mPictureWidth, > + mPictureHeight); mDisplayWidth, mDisplayHeight

(In reply to Ralph Giles (:rillian) from comment #5) > Comment on attachment 8573164 [details] [diff] [review] > Part1 - Extract display dimension from SPS NAL > > Review of attachment 8573164 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with comments addressed. > > ::: media/libstagefright/binding/H264.cpp > @@ +77,5 @@ > > chroma_format_idc = 1; > > + video_format = 5; > > + colour_primaries = 2; > > + transfer_characteristics = 2; > > + sample_ratio = 1.0; > > Where do all these default values come from? If you look in the header file, you'll see specification on what those values are assumed to be when they aren't defined. I put the initialisation in the constructor, the vui decoder was never called.. > > @@ +131,5 @@ > > +static int32_t > > +ConditionDimension(float aValue) > > +{ > > + // This will exclude NaNs and too-big values. > > + if (aValue > 1.0 && aValue <= INT32_MAX) > > Would be nice to use MAX_VIDEO_* from VideoUtils here, but I guess that > breaks the abstraction barriers. I think INT32_MAX is safe enough. I pretty much copied what the WMF decoder was doing and this code is a copy/paste from media utils. > > @@ +250,5 @@ > > + // Determine display size. > > + if (aDest.sample_ratio > 1.0) { > > + // Increase the intrinsic width > > + aDest.display_width = > > + ConditionDimension(aDest.sample_ratio * aDest.pic_width); > > pic_width first, please, to match the else clause. > > @@ +273,2 @@ > > > > + switch (aDest.aspect_ratio_idc) { > > Spec reference for this, please. E.g. > > // ITU T-REC-H.264 table E-1. The documentation for aspect_ratio_idc is already in the header.. > > @@ +371,5 @@ > > + // 160:99 > > + /* > > + 540x576 16:9 frame with horizontal overscan > > + */ > > + aDest.sample_ratio = 16.0 / 11.0; > > Cut and paste error? This should be 160. / 99. ? Comment doesn't match the > spec either. Looks like it :) > > What about 14-16? > > @@ +376,5 @@ > > + break; > > + case 255: > > + aDest.sar_width = aBr.ReadBits(16); > > + aDest.sar_height = aBr.ReadBits(16); > > + aDest.sample_ratio = float(aDest.sar_width) / float(aDest.sar_height); > > Please document here that you're relying on NaN handling downstream. Or > better yet, just don't update sample_ratio if sar_width or sar_height are > zero so no one has to worry about it. Good idea... Again, this is what the WMF decoder does, the values it gets when requesting the aspect ratio are actually sar_width and sar_height. > > @@ -272,5 @@ > > aDest.fixed_frame_rate_flag = aBr.ReadBit(); > > } > > - > > - bool hrd_present = false; > > - if (aBr.ReadBit()) { // nal_hrd_parameters_present_flag > > Do we no longer need to consume these bits to advance aBr's state? The bitreader content is emptied before returning. Actually, the main reason I dropped this code is because youtube encoded video have one less byte in their VUI than the spec specifies, so it would have asserted. If we ever plan to do our own h264 decoder, I'll re-add the handling of those :D

(In reply to Ralph Giles (:rillian) from comment #6) > Comment on attachment 8573166 [details] [diff] [review] > Part2. Use display dimension from SPS NAL > > Review of attachment 8573166 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with comments addressed. This will need some testing on wild content > though. I don't think it's a great candidate for getting mac dormant support > in 37. :/ > > ::: dom/media/fmp4/apple/AppleVDADecoder.cpp > @@ +57,5 @@ > > spsdata.pic_width && spsdata.pic_height) { > > mPictureWidth = spsdata.pic_width; > > mPictureHeight = spsdata.pic_height; > > + mDisplayWidth = spsdata.display_width; > > + mDisplayHeight = spsdata.display_height; > > So this is a major change in behaviour, right? This will unconditionally set > the sample aspect ratio to 1.0 if the SPS has no vui table, or if it doesn't > specify a ratio, overriding whatever's in the file header. How many wild > files have that kind of mismatch? That's right, a few videos. I have samples with no VUI, and all have the same picture size and dispay size. The thing is, we can't rely on what the metadata tells us. Not with AVC3 where an init segment is common to all video streams or annexB with resolution change mid-way. But so far, bar the odd rounding, of the 20-odd videos I tested, it all works. And also what the Mac decoder is doing now is exactly what the WMF decoder already does: it only relies on the SPS data and never the container metadata, and returning a ratio of 1.0 when none are defined is what WMF does. > > @@ +63,5 @@ > > // we need to queue that many frames in order to guarantee proper > > // pts frames ordering. Use a minimum of 4 to ensure proper playback of > > // non compliant videos. > > mMaxRefFrames = > > + std::min(std::max(mMaxRefFrames, spsdata.max_num_ref_frames + 1), 16u); > > Does this belong here? I don't see what it has to do with the rest of the > patch. It doesn't belong in this patch, but it had been in my todo list for a while. > > @@ +267,5 @@ > > info.mHasVideo = true; > > gfx::IntRect visible = gfx::IntRect(0, > > 0, > > + mPictureWidth, > > + mPictureHeight); > > mDisplayWidth, mDisplayHeight No. It was an error earlier on to use mDisplayWidth/Height there. Visible indicates the dimension displayed in the actual decoded frame. Now because it's an IOSurface it never mattered because those values aren't used, and instead the compositor uses the GetWidth/GetHeight() of MacIOSurface I hit an error with the FFmpeg code because of that, we would hit the error that the picture dimensions were bigger than YUV planes. So for clarity sake, i set the values to what they should always have been.

Actually, I'm going to change so Mac (and FFMpeg) decoder use the values in the VideoConfig structure and let the AVCC wrapper reads the SPS and update VideoConfig accordingly

Carrying r+, update to 2014/02 if ITU H.264 spec

Move display/picture size calculations from SPS into AVCC wrapper.

Comment on attachment 8573594 [details] [diff] [review] Part5. Move picture/display size calculations in AVCC wrapper Review of attachment 8573594 [details] [diff] [review]: ----------------------------------------------------------------- Yay consolidation.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/500b9af84860 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/25f20acf1138 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d24d798dcc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e557c24d9e2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b002c4cb0dc

https://hg.mozilla.org/mozilla-central/rev/500b9af84860 https://hg.mozilla.org/mozilla-central/rev/25f20acf1138 https://hg.mozilla.org/mozilla-central/rev/b9d24d798dcc https://hg.mozilla.org/mozilla-central/rev/4e557c24d9e2 https://hg.mozilla.org/mozilla-central/rev/7b002c4cb0dc

Comment on attachment 8573166 [details] [diff] [review] Part2. Use display dimension from SPS NAL Approval Request Comment [Feature/regressing bug #]: 1141914 [User impact if declined]: bug 1141914 already uplifted will not properly work [Describe test coverage new/current, TreeHerder]: This has been running in central for a week. [Risks and why]: Low, while this could affect all videos playing on mac outside of MSE, it's purely enforcing the H264/MP4 specs. Bug 1143516 will need to be uplifted. [String/UUID change made/needed]: None

The above is a blanker request for all patches in this bug

https://hg.mozilla.org/releases/mozilla-aurora/rev/2802cf435885 https://hg.mozilla.org/releases/mozilla-aurora/rev/8d8cd865b4d0 https://hg.mozilla.org/releases/mozilla-aurora/rev/20b06ff42ed2 https://hg.mozilla.org/releases/mozilla-aurora/rev/7b7b5e77e5e1 https://hg.mozilla.org/releases/mozilla-aurora/rev/d1e899435508