Closed Bug 1300693 Opened 5 years ago Closed 5 years ago

BBCiPlayer (TV) HTML5 broken in Nightly 51.0a1

Categories

(Core :: Audio/Video: Playback, defect, P1)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 + verified

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1300693 - BBCiPlayer (TV) HTML5 broken in Nightly 51.0a1.
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1300693 - adjust the indent.
58 bytes, text/x-review-board-request
jya
: review+
Details 
Got a regression issue that BBCiPlayer (TV) HTML5 broken in Nightly 51.0a1

After analysis, 

Since Bug 1291742 was intended to make sure the video format can be decoded, 

checking the extra data which should have SPS is essential.

In this case, BBC's content(vido/avc) did not contain SPS in its extradata which means it cannot pass the check [1].

H264::DecodeSPSFromExtraData return false...


Repro Step:
1. Link to http://www.bbc.co.uk/iplayer
2. Make sure you disable the flash player.
3. Got error when you wanna playback.


[1]
https://dxr.mozilla.org/mozilla-central/rev/dbe4b47941c7b3d6298a0ead5e40dd828096c808/dom/media/platforms/PDMFactory.cpp#112
BBC uses AVC3, which means that the extra data isn't provided in the metadata but is in-band.
This is what the H264 Converter does; it scan the actual stream and find the SPS in it
Comment on attachment 8788332 [details]
Bug 1300693 - BBCiPlayer (TV) HTML5 broken in Nightly 51.0a1.

https://reviewboard.mozilla.org/r/76852/#review74978

You'll also need to add the handling for AVC3 stream in H264Converter.
Attachment #8788332 - Flags: review?(jyavenard) → review-
Keywords: regression
Priority: -- → P1
Comment on attachment 8788332 [details]
Bug 1300693 - BBCiPlayer (TV) HTML5 broken in Nightly 51.0a1.

https://reviewboard.mozilla.org/r/76852/#review75016

::: dom/media/platforms/wrappers/H264Converter.cpp:140
(Diff revision 2)
>  {
>    if (mNeedAVCC && !mp4_demuxer::AnnexB::HasSPS(mCurrentConfig.mExtraData)) {
>      // nothing found yet, will try again later
>      return NS_ERROR_NOT_INITIALIZED;
>    }
> -  UpdateConfigFromExtraData(mCurrentConfig.mExtraData);
> +  auto spsdata = UpdateConfigFromExtraData(mCurrentConfig.mExtraData);

I find that doing like in the PDMFactory with simply having a temporary spsdsta and extract sps much cleaner than having to return it from an apparently unused function...

and you wouldnt have to change the header file either and everything would be more localised to a single location

::: dom/media/platforms/wrappers/H264Converter.cpp:143
(Diff revision 2)
>      return NS_ERROR_NOT_INITIALIZED;
>    }
> -  UpdateConfigFromExtraData(mCurrentConfig.mExtraData);
> +  auto spsdata = UpdateConfigFromExtraData(mCurrentConfig.mExtraData);
> +  // Do some format check here.
> +  // WMF H.264 Video Decoder and Apple ATDecoder do not support YUV444 format.
> +  if (spsdata.chroma_format_idc == 3 /*YUV444*/) {

if youre here, its because we have a SPS, so you could return early if the sps was invalid
Attachment #8788332 - Flags: review?(jyavenard) → review-
Comment on attachment 8788332 [details]
Bug 1300693 - BBCiPlayer (TV) HTML5 broken in Nightly 51.0a1.

https://reviewboard.mozilla.org/r/76852/#review75018
Attachment #8788332 - Flags: review- → review+
Duplicate of this bug: 1300823
Tracking 51+ for this regression. Popular video player.
tracking-firefox51: --- → +
Comment on attachment 8788332 [details]
Bug 1300693 - BBCiPlayer (TV) HTML5 broken in Nightly 51.0a1.

https://reviewboard.mozilla.org/r/76852/#review75360

::: dom/media/platforms/PDMFactory.cpp:126
(Diff revisions 2 - 3)
>  
>    SupportChecker::Result
>    Check()
>    {
>      for (auto& checker : mCheckerList) {
> -    auto result = checker();
> +      auto result = checker();

please separate this in a new patch... it's out of scope
Comment on attachment 8788332 [details]
Bug 1300693 - BBCiPlayer (TV) HTML5 broken in Nightly 51.0a1.

https://reviewboard.mozilla.org/r/76852/#review75362

::: dom/media/platforms/wrappers/H264Converter.cpp:152
(Diff revision 3)
> +      if (aDiagnostics) {
> +        aDiagnostics->SetVideoFormatNotSupport();
> +      }
> +      return NS_ERROR_FAILURE;
> +    }
> +  } else {

for now, you need to only reject it if mNeedAVCC is true.

so else if (mNeedAVCC) { }

otherwise, I'm afraid we could get regressions.

Make sure to test on windows :)
Thank you for your comments,

Attach treeherder link for reference,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33913f6c5051

I will also build a windows build locally and make sure this issue is fixed.

Thank you :)
I've verified the patch on windows and BBCiPlayer works.

The try result seems OK.

Please review another patch for adjust the indent.

Thank you~
Comment on attachment 8788774 [details]
Bug 1300693 - adjust the indent.

https://reviewboard.mozilla.org/r/77156/#review75440
Attachment #8788774 - Flags: review?(jyavenard) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/a89000231432
BBCiPlayer (TV) HTML5 broken in Nightly 51.0a1. r=jya
https://hg.mozilla.org/mozilla-central/rev/4d978f6af1cd
adjust the indent. r=jya
https://hg.mozilla.org/mozilla-central/rev/a89000231432
https://hg.mozilla.org/mozilla-central/rev/4d978f6af1cd
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: qe-verify+
I reproduced this issue using Fx 51.0a1, build ID: 20160905030222, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 51.0b10, build ID: 20161222080852, on Windows 10 x64.

Cheers!
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.