Telemetry: Collect h264 video profile and level via canPlayType and decoded SPS

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: miketaylr, Assigned: miketaylr)

Tracking

(Blocks 1 bug)

unspecified
mozilla38
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed)

Details

Attachments

(2 attachments, 10 obsolete attachments)

Assignee

Description

5 years ago
No description provided.
Assignee

Updated

5 years ago
Assignee: nobody → miket
Assignee

Comment 1

5 years ago
Posted patch 1125340-1.patch (obsolete) — Splinter Review
Assignee

Updated

5 years ago
Blocks: 1109958
Comment on attachment 8553941 [details] [diff] [review]
1125340-1.patch

Review of attachment 8553941 [details] [diff] [review]:
-----------------------------------------------------------------

Yes!
Attachment #8553941 - Flags: review?(cpearce) → review+
Assignee

Comment 3

5 years ago
Thanks Chris. :)
Assignee

Comment 4

5 years ago
ni? :bsmedberg for data collection approval.
Just thinking about this, the patch here will only report the profile and level that are passed into JavaScript's HTMLMediaElement.canPlayType(). We should also report the profile and level of the files that are actually *played*.

I think it makes sense to report these in a different histogram from what's passed to canPlayType, as often what's passed to canPlayType is totally bogus.

So we should also report that here:
https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Reader.cpp#440

You get the profile and level from the video.extra_data. You need to call H264::DecodeSPSFromExtraData() to parse the extra_data, and get the profile and level from the SPSData stuct that H264::DecodeSPSFromExtraData() writes into. JeanYves Avenard would be a good reviewer for that patch.

Here's an example to cargo-cult:
https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/apple/AppleVDADecoder.cpp#53
Flags: needinfo?(miket)
Assignee

Comment 6

5 years ago
Great suggestions, thanks Chris--I didn't realize that was only for canPlayType. Will revise the patch.

Comment 7

5 years ago
Why doesn't this histogram expire? Do you have plans to monitor it permanently? My understanding is that this is intended for a specific report that you are preparing, and the histogram should expire after that unless you have plans for ongoing reporting (and if so, I'd want to know who's implementing that dashboard and who's going to monitor it).

I'd like the histogram descriptions to be more detailed: that this is explicitly about what is passed to .canPlayType.

Please set feedback? on a new patch when that is done.

As an implementation nit, this patch doesn't bounds-check aProfile or aLevel before passing them to Accumulate. It should probably do that in some form, and perhaps have a special value for an invalid/unknown profile/level.
Flags: needinfo?(benjamin)
Assignee

Comment 8

4 years ago
Posted patch 1125340-2.patch (obsolete) — Splinter Review
(jumping back into this after PTO)

Thanks for the feedback Benjamin. Here's an updated patch that checks that we're recording expected values or 0 for unknown. I've also set expiration to 40--that should be long enough to collect some interesting data. If we decide it's useful to have on a more permanent basis, we can change that later (and our team is happy to build dashboards for that).

I'm going to file a separate bug to track histograms for profile/level of video as its played.
Attachment #8553941 - Attachment is obsolete: true
Assignee

Updated

4 years ago
See Also: → 1128569
Assignee

Updated

4 years ago
Summary: Telemetry: Collect h264 video profile and level → Telemetry: Collect h264 video profile and level via canPlayType

Comment 9

4 years ago
Comment on attachment 8557976 [details] [diff] [review]
1125340-2.patch

VIDEO_CANPLAYTYPE_H264_PROFILE says you have n_values=12, but when you accumulate it's 144. I don't understand how that fits together. r+ on the rest in any case, and I figure you can deal with that discrepancy in review.
Attachment #8557976 - Flags: feedback?(benjamin) → feedback+
Assignee

Comment 10

4 years ago
Sorry, that's a bit confusing. 

Here's the profile enum described in http://blog.pearce.org.nz/2013/11/what-does-h264avc1-codecs-parameters.html (which comes from some Windows 7 code).

enum eAVEncH264VProfile {
  eAVEncH264VProfile_unknown                    = 0,
  eAVEncH264VProfile_Simple                     = 66,
  eAVEncH264VProfile_Base                       = 66,
  eAVEncH264VProfile_Main                       = 77,
  eAVEncH264VProfile_High                       = 100,
  eAVEncH264VProfile_422                        = 122,
  eAVEncH264VProfile_High10                     = 110,
  eAVEncH264VProfile_444                        = 144,
  eAVEncH264VProfile_Extended                   = 88,
  eAVEncH264VProfile_ScalableBase               = 83,
  eAVEncH264VProfile_ScalableHigh               = 86,
  eAVEncH264VProfile_MultiviewHigh              = 118,
  eAVEncH264VProfile_StereoHigh                 = 128,
  eAVEncH264VProfile_ConstrainedBase            = 256,
  eAVEncH264VProfile_UCConstrainedHigh          = 257,
  eAVEncH264VProfile_UCScalableConstrainedBase  = 258,
  eAVEncH264VProfile_UCScalableConstrainedHigh  = 259
};

The profile is determined by two hex digits, so it can only ever be 255 decimal. So the last 4 in this enum can be ignored. That leaves us with 0, 66, 77, 83, 88, 86, 100, 110, 118, 122, 128, 144 as possible values to be recorded.
Assignee

Comment 11

4 years ago
> so it can only ever be 255 decimal

That is to say, it can only be between 0 and 255.
n_values determines the maximum value, so Accumulate expects exactly the numbers 0-11.
Assignee

Comment 13

4 years ago
Ah. My mistake. Will update the patch.
Assignee

Comment 14

4 years ago
Posted patch 1125340-3.patch (obsolete) — Splinter Review
Attachment #8557976 - Attachment is obsolete: true
Assignee

Comment 15

4 years ago
Posted patch 1125340-3-2.patch (obsolete) — Splinter Review
(Lumping this patch in this bug afterall...)

Hi Jean-Yves, would you mind reviewing this telemetry patch? It collects SPS profile_idc and level_idc when an MP4 is actually read, as opposed to what is gathered in canPlayType().
Assignee

Comment 16

4 years ago
Posted patch 1125340-3-2.patch (obsolete) — Splinter Review
Updated comment. 

One question I have is does it make sense to restrict what we're recording here for profile_idc, in the same way we do for canPlayType()? That is, we could in theory receive 259 (eAVEncH264VProfile_UCScalableConstrainedHigh). But I don't know if the constrained profiles are interesting to us.
Attachment #8558174 - Attachment is obsolete: true
Not sure if this is perfectly going to answer your question. But here it is:
right now, so long as it's an h264 profile we know about, we will accept it (that is level <= 5.1).
This is a blanket acceptance regardless of the platforms.

Knowing what codecs are being submitted is I think really good information (not that we could do much about it, seeing that if we don't support it, well, we just don't)
But for projects like OpenH264, knowing what's actually in use and were our effort should be targeted is a good thing.

So yes, I see use in the profile and idc level.
Comment on attachment 8558178 [details] [diff] [review]
1125340-3-2.patch

Review of attachment 8558178 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with comments addressed.

::: dom/media/fmp4/MP4Reader.cpp
@@ +475,5 @@
> +        spsdata.profile_idc && spsdata.level_idc) {
> +      // 144 is the highest meaningful decimal profile value that can be
> +      // represented as single hex byte, otherwise we collect 0 for unknown.
> +      Telemetry::Accumulate(Telemetry::VIDEO_DECODED_H264_SPS_PROFILE,
> +                            spsdata.profile_idc <= 144 ? spsdata.profile_idc : 0);

why limit to value <= 144 ? shouldn't this be 244 (High 4:4:4 Intra Profile)?

I would also record that fact that we have no extra_data as it indicates annexB.
In which case the SPS is inline.

We have routines to extract the SPS from an annexB AVC, but that won't be known at the time of readmetadata.

having said that, AVCC format is the most prevalent I would think (BBC appears to use annexB though).
Attachment #8558178 - Flags: review?(jyavenard) → review+
oh, while at it.

Can you also record SPS.max_num_ref_frames? This indicates how deep the h264 queue is going to be, and as such the minimum memory usage by the decoder.
Assignee

Comment 20

4 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #18)
> ::: dom/media/fmp4/MP4Reader.cpp
> @@ +475,5 @@
> > +        spsdata.profile_idc && spsdata.level_idc) {
> > +      // 144 is the highest meaningful decimal profile value that can be
> > +      // represented as single hex byte, otherwise we collect 0 for unknown.
> > +      Telemetry::Accumulate(Telemetry::VIDEO_DECODED_H264_SPS_PROFILE,
> > +                            spsdata.profile_idc <= 144 ? spsdata.profile_idc : 0);
> 
> why limit to value <= 144 ? shouldn't this be 244 (High 4:4:4 Intra Profile)?

Yeah, I was looking at some outdated profile data. Will update (and now I've got the latest version of the ITU-T H264 spec) in this patch and in the first. Thanks.

(In reply to Jean-Yves Avenard [:jya] from comment #19) 
> Can you also record SPS.max_num_ref_frames? This indicates how deep the h264
> queue is going to be, and as such the minimum memory usage by the decoder.

Sure, will add and ping for a final review.
Assignee

Comment 21

4 years ago
Posted patch 1125340-4.patch (obsolete) — Splinter Review
Updated patch addressing comments

I'm going to learn a bit more about SPS max_num_ref_frames (have added to https://wiki.mozilla.org/Compatibility/Telemetry) before adding a histogram for that. And read up on getting the inline SPS from Annex B AVCs.
Assignee

Comment 22

4 years ago
Posted patch 1125340-5.patch (obsolete) — Splinter Review
Attachment #8558103 - Attachment is obsolete: true
Attachment #8558178 - Attachment is obsolete: true
(In reply to Mike Taylor [:miketaylr] from comment #21)
> I'm going to learn a bit more about SPS max_num_ref_frames (have added to
> https://wiki.mozilla.org/Compatibility/Telemetry) before adding a histogram
> And read up on getting the inline SPS from Annex B AVCs.

There are two routines that will do what you want:
AnnexB::HasSPS() which takes either a MP4Sample or a ByteBuffer will tell you if the buffer has an SPS

and AnnexB::ExtractExtraData().

Now, you only want to do it once, and stop doing it once you've seen a SPS because otherwise that task is going to be done every time your passing a demuxed sample to the decoder.
You probably would do this in MP4Reader::Update() right after PopSample().

If you do it that way, no need to check during ReadMetaData then...
Assignee

Comment 25

4 years ago
Posted patch 1125340-6.patch (obsolete) — Splinter Review
Hey Jean-Yves, how does this look? 

The other approach I thought about was converting an AVCC sample in MP4Reader::Update to AnnexB with AnnexB::ConvertSampleToAnnexB(aSample)--and then always collecting telemetry from an AnnexB SPS. But it seems a little crazy (especially of it's less common).
Attachment #8558702 - Attachment is obsolete: true
Comment on attachment 8561693 [details] [diff] [review]
1125340-6.patch

Review of attachment 8561693 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments addressed.

::: dom/media/fmp4/MP4Reader.cpp
@@ +67,5 @@
>  }
>  #endif
>  
> +void
> +AccumulateSPSTelemetry(nsRefPtr<ByteBuffer> aExtradata)

ByteBuffer* would be better, prevent unnecessary nsRefPtr construction and then calls to get() later

@@ +81,5 @@
> +    // otherwise collect 0 for unknown level.
> +    Telemetry::Accumulate(Telemetry::VIDEO_DECODED_H264_SPS_LEVEL,
> +                          (spsdata.level_idc >= 10 && spsdata.level_idc <= 52) ?
> +                          spsdata.level_idc : 0);
> +  }

you test if you could decode the SPS, but set mFoundSPSForTelemetry regardless.

returning a bool and then set you could do mFoundSPSForTelemetry = AccumulateSPSTelemetry(...)

not going to make a difference for AVCC, but for annexB it would
Attachment #8561693 - Flags: review?(jyavenard) → review+
oh, and what happened to max_num_ref_frames?
Assignee

Comment 28

4 years ago
Posted patch 1125340-7.patch (obsolete) — Splinter Review
Updated patch with comments addressed (and thanks for the suggestions!)
Attachment #8561693 - Attachment is obsolete: true
Assignee

Comment 29

4 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #27)
> oh, and what happened to max_num_ref_frames?

I filed Bug 1131706 to track that -- I also want to grab constaint flags. Let's land this and I'll start on that one.
Assignee

Updated

4 years ago
Summary: Telemetry: Collect h264 video profile and level via canPlayType → Telemetry: Collect h264 video profile and level via canPlayType and dedcoded SPS
Assignee

Comment 30

4 years ago
Try is (mostly) green, with the exception of two known timeouts: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f7f4e6a561f
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1960750&repo=fx-team
Flags: needinfo?(miket)
Whiteboard: [fixed-in-fx-team]
Comment on attachment 8562205 [details] [diff] [review]
1125340-7.patch

Review of attachment 8562205 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/fmp4/MP4Reader.cpp
@@ +721,5 @@
> +    if (!mFoundSPSForTelemetry && AnnexB::HasSPS(sample)) {
> +      nsRefPtr<ByteBuffer> extradata = AnnexB::ExtractExtraData(sample);
> +      mFoundSPSForTelemetry = AccumulateSPSTelemetry(extradata);
> +    }
> +

Sorry I should have picked that up.
PopSample returns nullptr when it reaches EOS.

so it should be:
if (sample && !mFoundSPSForTelemetry && AnnexB::HasSPS(sample)) {

HasSPS does no check whatsoever, maybe it should...
Assignee

Comment 34

4 years ago
Oops. Will fix up today.
Assignee

Comment 35

4 years ago
Attachment #8558699 - Attachment is obsolete: true
Assignee

Comment 36

4 years ago
Attachment #8562205 - Attachment is obsolete: true
Assignee

Comment 37

4 years ago
Posted patch 1125340-10.patch (obsolete) — Splinter Review
I went ahead and added an early return to AnnexB::HasSPS.
Assignee

Comment 38

4 years ago
Another try run, let's test on Windows and OSX just in case™ since Linux was green on the last.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cd5c486de1f
Assignee

Updated

4 years ago
Summary: Telemetry: Collect h264 video profile and level via canPlayType and dedcoded SPS → Telemetry: Collect h264 video profile and level via canPlayType and decoded SPS
Attachment #8562867 - Flags: review?(jyavenard) → review+
Comment on attachment 8562870 [details] [diff] [review]
1125340-10.patch

Review of attachment 8562870 [details] [diff] [review]:
-----------------------------------------------------------------

None of the annexB/h264 check the validity of the argument provided. The assumption has always been that it is passed a valid pointer to a sample
I'm happy if it now does, but it has to be done consistently, and there's more work required for it to do so. There are a few asserts to remove as well and handle gracefully.
Attachment #8562870 - Flags: review?(jyavenard) → review-
Assignee

Comment 40

4 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #39)
> None of the annexB/h264 check the validity of the argument provided. The
> assumption has always been that it is passed a valid pointer to a sample
> I'm happy if it now does, but it has to be done consistently, and there's
> more work required for it to do so. There are a few asserts to remove as
> well and handle gracefully.

OK, fair enough. Any objections to landing the other two telemetry patches (without touching AnnexB::HasSPS)?
Those two others are fine, I r+ them :)

Being anal I would have swapped the two values tested in that if. Testing that we had past telemetry first as it's the most likely to be true, and as such abort the test early and save you a load and a comparison :)
Flags: needinfo?(jyavenard)
Assignee

Comment 42

4 years ago
Comment on attachment 8562870 [details] [diff] [review]
1125340-10.patch

Alright, let's kill this patch.

Ah yeah, makes sense to swap the tests in the if... let me take care of that in Bug 1131706. Thanks again for the help!
Attachment #8562870 - Attachment is obsolete: true
Assignee

Comment 43

4 years ago
Try run from Comment 38 is green on Windows and Mac, let's try again.
Keywords: checkin-needed
Depends on: 1132158
https://hg.mozilla.org/mozilla-central/rev/2ec6877bd567
https://hg.mozilla.org/mozilla-central/rev/bfc486042129
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38
Comment on attachment 8562865 [details] [diff] [review]
1125340-8.patch

Requesting 37 uplift of both patches in this bug. It would be nice to get this data sooner to help with MSE support analysis.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less data available during feature rollout.
[Describe test coverage new/current, TreeHerder]: Landed on 38 some time ago.
[Risks and why]: Risk should be low. Adds new telemetry probes to mp4 video decoding.
[String/UUID change made/needed]: None.
Attachment #8562865 - Flags: approval-mozilla-beta?
Comment on attachment 8562865 [details] [diff] [review]
1125340-8.patch

Safe Telemetry probe addition for a feature shipping in 37. Beta+
Attachment #8562865 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.