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

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Mike Taylor [:miketaylr]]

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).

Comment 1

[Mike Taylor [:miketaylr]]

I went ahead and added a probe for max_num_ref_frames as well (rather than in Bug 1131706).

Comment 2

[Mike Taylor [:miketaylr]]

Attachment: 1136877-1.patch

Comment 3

[Mike Taylor [:miketaylr]]

Attachment: 1136877-2.patch

Comment 4

[Mike Taylor [:miketaylr]]

Attachment: 1136877-3.patch

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.

Comment 5

[Mike Taylor [:miketaylr]]

"These patches add probes for collecting h264 levels" 

s/levels/constraint flags/

Comment 6

[Jean-Yves Avenard [:jya]]

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.

Comment 7

[Jean-Yves Avenard [:jya]]

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

Comment 8

[Benjamin Smedberg]

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.

Comment 9

[Mike Taylor [:miketaylr]]

(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.

Comment 10

[Mike Taylor [:miketaylr]]

(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.

Comment 11

[Mike Taylor [:miketaylr]]

(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.

Comment 12

[Mike Taylor [:miketaylr]]

Attachment: 1136877-1-2.patch

Updating commit to show r=jya (carrying forward r+)

Comment 13

[Mike Taylor [:miketaylr]]

Attachment: 1136877-2-2.patch

OK, I've nuked the enum and am just bitwise ORing the flags into constraints and passing that to Telemetry.

Comment 14

[Mike Taylor [:miketaylr]]

Comment on attachment 8570547 [details] [diff] [review]
1136877-2.patch

Removing feedback request from Vladan since I removed the enum.

Comment 15

[Mike Taylor [:miketaylr]]

Attachment: 1136877-3-2.patch

Updated to use std::min and collect anything above 16 as invalid.

Comment 16

[Jean-Yves Avenard [:jya]]

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

Comment 17

[Mike Taylor [:miketaylr]]

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.

Comment 18

[Mike Taylor [:miketaylr]]

Let's see if try explodes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5aa493e093b5

Comment 19

[Mike Taylor [:miketaylr]]

Attachment: 1136877-1-3.patch

Converting patches to hg format and carrying forward r+ from :jya.

Comment 20

[Mike Taylor [:miketaylr]]

Attachment: 1136877-2-3.patch

Comment 21

[Mike Taylor [:miketaylr]]

Attachment: 1136877-3-3.patch

Comment 22

[Mike Taylor [:miketaylr]]

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

Comment 23

[Mike Taylor [:miketaylr]]

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?

Comment 24

[Jean-Yves Avenard [:jya]]

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);

Comment 25

[Mike Taylor [:miketaylr]]

Gotcha, thanks.

Comment 26

[Mike Taylor [:miketaylr]]

Attachment: 1136877-2-4.patch

Comment 27

[Mike Taylor [:miketaylr]]

OK, that made Windows much happier. Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f28dfc1c20e

Comment 28

[Carsten Book [:Tomcat]]

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!

Comment 29

[Mike Taylor [:miketaylr]]

Attachment: 1136877-2-5.patch

Ah yep, tiny amount of bitrot in the 2nd and 3rd patch. Rebased and ready to go.

Comment 30

[Mike Taylor [:miketaylr]]

Attachment: 1136877-3-4.patch

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6895878594b2
https://hg.mozilla.org/integration/fx-team/rev/f07b1abfeaab
https://hg.mozilla.org/integration/fx-team/rev/912bde581e6e

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6895878594b2
https://hg.mozilla.org/mozilla-central/rev/f07b1abfeaab
https://hg.mozilla.org/mozilla-central/rev/912bde581e6e

Comment 33

[Ralph Giles (:rillian)]

Attachment: backport of part 2 to 37/38

Comment 34

[Ralph Giles (:rillian)]

Attachment: backport of part 3 to 37/38

Comment 35

[Ralph Giles (:rillian)]

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.

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

Approving (and tracking) for 37/38 since this info will be useful sooner rather than later.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for beta.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/017aeb6767db
https://hg.mozilla.org/releases/mozilla-aurora/rev/6936b9b49996
https://hg.mozilla.org/releases/mozilla-aurora/rev/3b60141915ae

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3aa618647813
https://hg.mozilla.org/releases/mozilla-beta/rev/027a0ac06377
https://hg.mozilla.org/releases/mozilla-beta/rev/99fe61cb246f