Closed Bug 1136877 Opened 5 years ago Closed 5 years ago

Telemetry: Collect h264 video constraint flags via canPlayType and decoded SPS

Categories

(Toolkit :: Telemetry, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: miketaylr, Assigned: miketaylr)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 9 obsolete files)

2.92 KB, patch
miketaylr
: review+
Details | Diff | Splinter Review
3.94 KB, patch
miketaylr
: review+
Details | Diff | Splinter Review
3.06 KB, patch
miketaylr
: review+
Details | Diff | Splinter Review
3.80 KB, patch
Details | Diff | Splinter Review
2.98 KB, patch
Details | Diff | Splinter Review
We're going to grab the rest of the flags in Bug 1131706, but let's split out the constraint flags so we can compare canPlayType to the decoded SPS (like we do with profile and level).
Assignee: nobody → miket
Blocks: 1109958
I went ahead and added a probe for max_num_ref_frames as well (rather than in Bug 1131706).
Attached patch 1136877-1.patch (obsolete) — Splinter Review
Attached patch 1136877-2.patch (obsolete) — Splinter Review
Attached patch 1136877-3.patch (obsolete) — Splinter Review
Requesting feedback from bsmedberg for data collection approval. These patches add probes for collecting h264 levels from canPlayType and the decoded SPS, as well as max_num_ref_frames.
"These patches add probes for collecting h264 levels" 

s/levels/constraint flags/
Attachment #8570546 - Flags: review?(jyavenard) → review+
Comment on attachment 8570547 [details] [diff] [review]
1136877-2.patch

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

::: dom/media/fmp4/MP4Reader.cpp
@@ +75,5 @@
> +  CONSTRAINT_SET2_FLAG = 1 << 5,
> +  CONSTRAINT_SET1_FLAG = 1 << 6,
> +  CONSTRAINT_SET0_FLAG = 1 << 7
> +};
> +

I don't understand why flag 0 suddenly becomes bit 7 and so on.

What's the logic behind this?

@@ +102,5 @@
> +      constraints |= CONSTRAINT_SET5_FLAG;
> +    }
> +    // Record which constraint_set flag is set
> +    Telemetry::Accumulate(Telemetry::VIDEO_DECODED_H264_SPS_CONSTRAINT_SET_FLAG,
> +                          (constraints >= 4 && constraints <= 128) ? constraints : 0);

I also don't get why you would need to check that it's <= 128 ; that would be testing that only flag 5 is set and nothing else. I see no such rule in the ITU spec.

constraint_setX_flag is already extracted from a bit mask.

So simply storing :
spsdata.constraint_set0_flag | spsdata.constraint_set1_flag << 1 | spsdata.constraint_set2_flag << 2 | spsdata.constraint_set3_flag << 3 | spsdata.constraint_set4_flag << 4 | | spsdata.constraint_set5_flag << 5

and store the data as-is, no test nothing.
Attachment #8570547 - Flags: review?(jyavenard) → review-
Comment on attachment 8570551 [details] [diff] [review]
1136877-3.patch

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

::: dom/media/fmp4/MP4Reader.cpp
@@ +108,5 @@
> +    if (spsdata.max_num_ref_frames) {
> +      // max_num_ref_frames should be between 0 and 16, but here we
> +      // consider anything higher than 16 as 16.
> +      Telemetry::Accumulate(Telemetry::VIDEO_H264_SPS_MAX_NUM_REF_FRAMES,
> +                            (spsdata.max_num_ref_frames >= 0 && spsdata.max_num_ref_frames <= 15) ?

nit: formatting to 80 colums

use std::min(spsdata.max_num_ref_frames, 16) (with matching #include <algorithm>)

Having said that, I don't see the point of storing all value greater than 16 as 16 gives us. We lose the information that the encoded value was incorrect to start with.

I would much prefer: std::min(spsdata.max_num_ref_frames, 17) at least we know that if we have 17 it tells us that something was wrong. 16 is a perfectly in-spec value
Attachment #8570551 - Flags: review?(jyavenard) → review-
Attachment #8570546 - Flags: feedback?(benjamin) → feedback+
Comment on attachment 8570547 [details] [diff] [review]
1136877-2.patch

Vladan, is this bit pattern a reasonable way to do things in telemetry, or is there a better way?

I'm not 100% sure on what the final question we're trying to answer is. A set of booleans might be just as good here, and would show up more usefully on the automated dashboard.
Attachment #8570547 - Flags: feedback?(benjamin) → feedback?(vdjeric)
Attachment #8570551 - Flags: feedback?(benjamin) → feedback+
(In reply to Jean-Yves Avenard [:jya] from comment #6)
> I don't understand why flag 0 suddenly becomes bit 7 and so on.
> 
> What's the logic behind this?

The logic is to keep the values directly comparable between the VIDEO_CANPLAYTYPE_H264_CONSTRAINT_SET_FLAG and VIDEO_DECODED_H264_SPS_CONSTRAINT_SET_FLAG histograms. That way we can compare them vis-a-vis like http://miketaylr.github.io/compat-telemetry-dashboards/h264level.html.

As far as I understand from http://tools.ietf.org/html/rfc6381#section-3.3, for a given codecs value like avc1.PPCCLL, the CC byte is arranged like so: 0123 45XX (with the XX being reserved_zero_2bits). That's why flag 0 is 7, etc.

However, I can just massage the data in JS when making the dashboard.

> @@ +102,5 @@
> > +      constraints |= CONSTRAINT_SET5_FLAG;
> > +    }
> > +    // Record which constraint_set flag is set
> > +    Telemetry::Accumulate(Telemetry::VIDEO_DECODED_H264_SPS_CONSTRAINT_SET_FLAG,
> > +                          (constraints >= 4 && constraints <= 128) ? constraints : 0);
> 
> I also don't get why you would need to check that it's <= 128 ; that would
> be testing that only flag 5 is set and nothing else. I see no such rule in
> the ITU spec.

Hmm, yeah. It doesn't make a whole lot of sense.
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Comment on attachment 8570551 [details] [diff] [review]
> 1136877-3.patch
> I would much prefer: std::min(spsdata.max_num_ref_frames, 17) at least we
> know that if we have 17 it tells us that something was wrong. 16 is a
> perfectly in-spec value

Thanks, I misunderstood your last email--my bad. Will change to 17+ and use that as an invalid bucket.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Comment on attachment 8570547 [details] [diff] [review]
> 1136877-2.patch
> 
> Vladan, is this bit pattern a reasonable way to do things in telemetry, or
> is there a better way?

Yes, I'm also interested to know the best way. Bug 1131706 is about collecting telemetry on 17 more flags from the SPS.
 
> I'm not 100% sure on what the final question we're trying to answer is. A
> set of booleans might be just as good here, and would show up more usefully
> on the automated dashboard.

The question is: "What are the flags set in the H264 SPS in videos in the wild", which is useful to understand what features are needed to play a video file and what features matter for open264 development, for example.
Attached patch 1136877-1-2.patch (obsolete) — Splinter Review
Updating commit to show r=jya (carrying forward r+)
Attachment #8570546 - Attachment is obsolete: true
Attached patch 1136877-2-2.patch (obsolete) — Splinter Review
OK, I've nuked the enum and am just bitwise ORing the flags into constraints and passing that to Telemetry.
Comment on attachment 8570547 [details] [diff] [review]
1136877-2.patch

Removing feedback request from Vladan since I removed the enum.
Attachment #8570547 - Attachment is obsolete: true
Attached patch 1136877-3-2.patch (obsolete) — Splinter Review
Updated to use std::min and collect anything above 16 as invalid.
Attachment #8570551 - Attachment is obsolete: true
Attachment #8571746 - Flags: review?(jyavenard) → review+
Comment on attachment 8571747 [details] [diff] [review]
1136877-3-2.patch

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

r=me with nit addressed.

::: dom/media/fmp4/MP4Reader.cpp
@@ +93,5 @@
>                            spsdata.level_idc : 0);
>  
> +    // max_num_ref_frames should be between 0 and 16, anything larger will
> +    // be treated as invalid.
> +    unsigned int invalid_num = 17;

const uint32_t

though, personally, I would have simply used 17 directly in the line below
Attachment #8571747 - Flags: review?(jyavenard) → review+
The compiler got sad when I originally had 17 in std::min:

> note: candidate template ignored: deduced conflicting types for parameter '_Tp' ('unsigned int' vs. 'int')
> 0:42.09     min(const _Tp& __a, const _Tp& __b)

The compiler is much happier with 17u (just learned about unsigned integer suffixes, neat), so I've changed it to that.
Converting patches to hg format and carrying forward r+ from :jya.
Attachment #8571745 - Attachment is obsolete: true
Attached patch 1136877-2-3.patch (obsolete) — Splinter Review
Attachment #8571746 - Attachment is obsolete: true
Attached patch 1136877-3-3.patch (obsolete) — Splinter Review
Attachment #8571747 - Attachment is obsolete: true
So Windows failed with the following:

> 10:09:25 ERROR - Automation Error: hg not responding 
> 10:09:26 INFO - CalledProcessError: Command '['hg', '--config', 'extensions.purge=', 'purge', '-a', '--all', 'c:\\builds\\moz2_slave\\try-w32-d-00000000000000000000\\build\\src']' returned non-zero exit status 255 
> 10:53:14 INFO - c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/dom/media/fmp4/MP4Reader.cpp(77) : error C2220: warning treated as error - no 'object' file generated
> 10:53:14 INFO - Warning: C4805 in c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\dom\media\fmp4\MP4Reader.cpp: '|' : unsafe mix of type 'bool' and type 'int' in operation

I guess Windows isn't happy about shifting bool bits into a uint8_t.

And OSX apparently just died due to a timeout. I guess Bug 1138955.

> command timed out: 10800 seconds without output running ['/tools/buildbot/bin/python', 'scripts/scripts/fx_desktop_build.py', '--config', 'builds/releng_base_mac_64_builds.py', '--custom-build-variant-cfg', 'debug', '--branch', 'try', '--build-pool', 'production'], attempting to kill
Jean-Yves, about the "unsafe mix of type 'bool' and type 'int' in operation" warning/error, does it seem fine to just remove the uint8_t constraints variable and pass the value directly to Telemetry? Or is there some more idiomatic way of doing this?
You will have the same warnings.

probably the simplest would be (and easiest to read for sure).

+  if (H264::DecodeSPSFromExtraData(aExtradata, spsdata)) {
+    uint8_t constraints = (spsdata.constraint_set0_flag ? (1 << 0) : 0) |
+                          (spsdata.constraint_set1_flag ? (1 << 1) : 0) |
+                          (spsdata.constraint_set2_flag ? (1 << 2) : 0) |
+                          (spsdata.constraint_set3_flag ? (1 << 3) : 0) |
+                          (spsdata.constraint_set4_flag ? (1 << 4) : 0) |
+                          (spsdata.constraint_set5_flag ? (1 << 5) : 0);
Flags: needinfo?(jyavenard)
Gotcha, thanks.
Attached patch 1136877-2-4.patch (obsolete) — Splinter Review
Attachment #8572036 - Attachment is obsolete: true
OK, that made Windows much happier. Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f28dfc1c20e
Keywords: checkin-needed
Hi Mike, the 2nd patch failed to apply:

patching file dom/media/fmp4/MP4Reader.cpp
Hunk #1 FAILED at 14
1 out of 2 hunks FAILED -- saving rejects to file dom/media/fmp4/MP4Reader.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 1136877-3-3.patch

could you take a look, thanks!
Flags: needinfo?(miket)
Keywords: checkin-needed
Ah yep, tiny amount of bitrot in the 2nd and 3rd patch. Rebased and ready to go.
Attachment #8572420 - Attachment is obsolete: true
Attachment #8572037 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 8577006 [details] [diff] [review]
backport of part 3 to 37/38

Requesting 38 and optionally 37 uplift of all three patches on this bug. We'd like to have this data start sooner to support MSE analysis.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less data available during feature release.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This adds new telemetry taps. Risk looks low to me.
[String/UUID change made/needed]: None.
Attachment #8577006 - Flags: approval-mozilla-beta?
Attachment #8577006 - Flags: approval-mozilla-aurora?
Attachment #8577006 - Flags: approval-mozilla-beta?
Attachment #8577006 - Flags: approval-mozilla-beta+
Attachment #8577006 - Flags: approval-mozilla-aurora?
Attachment #8577006 - Flags: approval-mozilla-aurora+
Approving (and tracking) for 37/38 since this info will be useful sooner rather than later.
Needs rebasing for beta.
Flags: needinfo?(miket)
Flags: needinfo?(miket)
Blocks: 1169473
You need to log in before you can comment on or make changes to this bug.