Move keyframe query into VPXDecoder

RESOLVED FIXED in Firefox 55

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

6 months ago
WebMDemuxer calls libvpx directly to detect keyframes. I'd like to move this into a method on VPXDecoder to simplify adding other decoders and to better isolate the code.
(Assignee)

Updated

6 months ago
Assignee: nobody → giles
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8861205 - Flags: review?(jyavenard)

Comment 6

6 months ago
mozreview-review
Comment on attachment 8861204 [details]
Bug 1358662 - Store LastSeenFrame dimensions as an nsIntSize.

https://reviewboard.mozilla.org/r/133160/#review136124
Attachment #8861204 - Flags: review?(jyavenard) → review+

Comment 7

6 months ago
mozreview-review
Comment on attachment 8861204 [details]
Bug 1358662 - Store LastSeenFrame dimensions as an nsIntSize.

https://reviewboard.mozilla.org/r/133160/#review136126

::: commit-message-62a2d:3
(Diff revision 1)
> +Bug 1358662 - Store LastSeenFrame dimensions as an nsIntSize. r?jya
> +
> +This simplifies the comparison and and update logic.

s/and and/and

Comment 8

6 months ago
mozreview-review
Comment on attachment 8861205 [details]
Bug 1358662 - Store VPXDecoder codec as an enum.

https://reviewboard.mozilla.org/r/133162/#review136128
Attachment #8861205 - Flags: review?(jyavenard) → review+

Comment 9

6 months ago
mozreview-review
Comment on attachment 8861206 [details]
Bug 1358662 - Add Span support to MediaRawData.

https://reviewboard.mozilla.org/r/133164/#review136130

::: commit-message-228af:1
(Diff revision 1)
> +Bug 1358662 - Add Span support to RawMediaData. r?jya

MediaRawData

::: dom/media/MediaData.h:664
(Diff revision 1)
>      return sizeof(*this)
>             + mBuffer.ComputedSizeOfExcludingThis()
>             + mAlphaBuffer.ComputedSizeOfExcludingThis();
>    }
> +  // Access the buffer as a Span.
> +  operator Span<const uint8_t>() { return MakeSpan(Data(), Size()); }

what's span ? (answering my own question after reading comments in Span.h)

::: dom/media/MediaData.h:664
(Diff revision 1)
>      return sizeof(*this)
>             + mBuffer.ComputedSizeOfExcludingThis()
>             + mAlphaBuffer.ComputedSizeOfExcludingThis();
>    }
> +  // Access the buffer as a Span.
> +  operator Span<const uint8_t>() { return MakeSpan(Data(), Size()); }

Surely we need to include a header to support this type and MakeSpan?
#include "mozilla/Span.h" maybe?
Attachment #8861206 - Flags: review?(jyavenard) → review+

Comment 10

6 months ago
mozreview-review
Comment on attachment 8861207 [details]
Bug 1358662 - Implement keyframe and framesize VPXDecoder helpers.

https://reviewboard.mozilla.org/r/133166/#review136136
Attachment #8861207 - Flags: review?(jyavenard) → review+

Comment 11

6 months ago
mozreview-review
Comment on attachment 8861208 [details]
Bug 1358662 - Call VPXDecoder libvpx wrappers for WebM.

https://reviewboard.mozilla.org/r/133168/#review136140

::: dom/media/webm/WebMDemuxer.cpp:677
(Diff revision 1)
>        if (packetEncryption == NESTEGG_PACKET_HAS_SIGNAL_BYTE_ENCRYPTED) {
>          // Packet is encrypted, can't peek, use packet info
>          isKeyframe = nestegg_packet_has_keyframe(holder->Packet())
>                       == NESTEGG_PACKET_HAS_KEYFRAME_TRUE;
>        } else {
> -        vpx_codec_stream_info_t si;
> +        auto sample = MakeSpan(data, length);

Have we calculated the overhead of using bounded memory access ?

while it's not a critical path, I still think we should worry about those.

::: dom/media/webm/WebMDemuxer.cpp:692
(Diff revision 1)
>          }
> -        isKeyframe = si.is_kf;
>          if (isKeyframe) {
> -          // We only look for resolution changes on keyframes for both VP8 and
> -          // VP9. Other resolution changes are invalid.
> -          auto dimensions = nsIntSize(si.w, si.h);
> +          // For both VP8 and VP9, we only look for resolution changes
> +          // on keyframes. Other resolution changes are invalid.
> +          auto codec = mVideoCodec == NESTEGG_CODEC_VP8 ?

unary operator is to be put underneath the operand

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators
"Overlong expressions not joined by && and || should break so the operator starts on the second line and starts in the same column as the start of the expression in the first line. This applies to ?"

so ? is to be aligned on the following line:
auto codec = mVideoCodec == NESTEGG_CODEC_VP8
             ? VPXDecoder::Codec::VP8
             : VPXDecoder::Codec::VP9;
Attachment #8861208 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 12

6 months ago
mozreview-review-reply
Comment on attachment 8861206 [details]
Bug 1358662 - Add Span support to MediaRawData.

https://reviewboard.mozilla.org/r/133164/#review136130

> MediaRawData

Oops, thanks.

> what's span ? (answering my own question after reading comments in Span.h)

It's an object wrapping a pointer and length together.

> Surely we need to include a header to support this type and MakeSpan?
> #include "mozilla/Span.h" maybe?

Good point. I've added the extra include.
(Assignee)

Comment 13

6 months ago
mozreview-review-reply
Comment on attachment 8861208 [details]
Bug 1358662 - Call VPXDecoder libvpx wrappers for WebM.

https://reviewboard.mozilla.org/r/133168/#review136140

> Have we calculated the overhead of using bounded memory access ?
> 
> while it's not a critical path, I still think we should worry about those.

I don't think we use any of the bounds-checking accessors here. The VPXDecoder methods just turn it back into a base pointer and length for passing to the libvpx C API. I was more worried about struct allocation and passing if nothing gets inlined, but the arguments should be the same size as before and, as you say, something that happens once per frame (twice per keyframe) isn't on a critical path.

I wanted Span here because it's conceptually cleaner, make the function prototypes shorter, and because tying the bounds to the pointer type give some support for correct handling just from style, even if we don't use the runtime bounds checking on random element access.

> unary operator is to be put underneath the operand
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators
> "Overlong expressions not joined by && and || should break so the operator starts on the second line and starts in the same column as the start of the expression in the first line. This applies to ?"
> 
> so ? is to be aligned on the following line:
> auto codec = mVideoCodec == NESTEGG_CODEC_VP8
>              ? VPXDecoder::Codec::VP8
>              : VPXDecoder::Codec::VP9;

Fixed, thanks!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

6 months ago
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/555b0322f776
Store LastSeenFrame dimensions as an nsIntSize. r=jya
https://hg.mozilla.org/integration/autoland/rev/20e051241152
Store VPXDecoder codec as an enum. r=jya
https://hg.mozilla.org/integration/autoland/rev/a3284ad8a14c
Add Span support to MediaRawData. r=jya
https://hg.mozilla.org/integration/autoland/rev/5da7e883ba70
Implement keyframe and framesize VPXDecoder helpers. r=jya
https://hg.mozilla.org/integration/autoland/rev/377615a0cf70
Call VPXDecoder libvpx wrappers for WebM. r=jya
Backed out for failing mda, crashtests and reftests:

https://hg.mozilla.org/integration/autoland/rev/d77b4701d794b610c3067ff0ac22bdc663ebf017
https://hg.mozilla.org/integration/autoland/rev/c10e88827d3994f6c71ea9967135107887afb043
https://hg.mozilla.org/integration/autoland/rev/57ce01b918d58bc62f459b7ebfcfcade31a81301
https://hg.mozilla.org/integration/autoland/rev/d5b2e40b8e183add9223ce2de09e68cbf3bf0521
https://hg.mozilla.org/integration/autoland/rev/c1fd7978446db3d1799dccebaf5984fcc9dcd552

Push for which many of the failing tests ran: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4f091d53060cc9857668bee0640a44fb1654c8c9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Android 4.3: mda: test_EndedEvent.html, test_FrameSelection.html
OS X 10.10 debug: mda: test_BufferingWait.html

Linux x64 asan&pgo:
crashtest: video-replay-after-audio-end.html
reftest: bug686957.html
Flags: needinfo?(giles)
And some talos (g4) tests: https://treeherder.mozilla.org/logviewer.html#?job_id=94175487&repo=autoland
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

6 months ago
I believe the issue here was failing to initialize the vpx_codec_stream_info struct passed to libvpx in the `VPXDecoder::IsKeyframe()` implementation. The libvpx function verifies the `sz` member before filling out the rest of the struct. Since we weren't setting it, no keyframes would be detected in some cases, causing timeouts on WebM tests, and probably also the talos regression.

The idea seems to have been that different codec implementations under the generic `vpx_codec_peek_stream_info` could extend this struct, so it's important to check the size in the dispatch code. However, no initializer was ever provided, so we have to do that ourselves. :P

Anyway, I'll let a few remaining try jobs cycle and then try landing again. https://treeherder.mozilla.org/#/jobs?repo=try&revision=27e9709b542d0c8e1d3e7a21e1f1d884c288b17f
Flags: needinfo?(giles)
(Assignee)

Comment 28

6 months ago
Still a few media mochitest timeouts, but I think they might be normal intermittents. I'll defer to the sheriffs' judgement.

Comment 29

6 months ago
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18aa5be434c9
Store LastSeenFrame dimensions as an nsIntSize. r=jya
https://hg.mozilla.org/integration/autoland/rev/f5f8b67e76bf
Store VPXDecoder codec as an enum. r=jya
https://hg.mozilla.org/integration/autoland/rev/2b2e1506e468
Add Span support to MediaRawData. r=jya
https://hg.mozilla.org/integration/autoland/rev/fd20fae8e972
Implement keyframe and framesize VPXDecoder helpers. r=jya
https://hg.mozilla.org/integration/autoland/rev/7764e3cf22a8
Call VPXDecoder libvpx wrappers for WebM. r=jya

Comment 30

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/18aa5be434c9
https://hg.mozilla.org/mozilla-central/rev/f5f8b67e76bf
https://hg.mozilla.org/mozilla-central/rev/2b2e1506e468
https://hg.mozilla.org/mozilla-central/rev/fd20fae8e972
https://hg.mozilla.org/mozilla-central/rev/7764e3cf22a8
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.