Closed Bug 1358662 Opened 8 years ago Closed 8 years ago

Move keyframe query into VPXDecoder

Categories

(Core :: Audio/Video: Playback, enhancement)

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: rillian, Assigned: rillian)

References

Details

Attachments

(5 files)

Bug 1358662 - Store LastSeenFrame dimensions as an nsIntSize.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1358662 - Store VPXDecoder codec as an enum.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1358662 - Add Span support to MediaRawData.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1358662 - Implement keyframe and framesize VPXDecoder helpers.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1358662 - Call VPXDecoder libvpx wrappers for WebM.
59 bytes, text/x-review-board-request
jya
: review+
Details
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: nobody → giles
Attachment #8861205 - Flags: review?(jyavenard)
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 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 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 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 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 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+
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.
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!
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
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)
Still a few media mochitest timeouts, but I think they might be normal intermittents. I'll defer to the sheriffs' judgement.
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: