Closed Bug 1337559 Opened 8 years ago Closed 8 years ago

Don't wait until all drained surfaces have been decoded/converted before resolving the drain promise

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Platform:
Unspecified
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(5 files)

Bug 1337559: [WMF] P1. Avoid draining the audio decoder twice in a row.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1337559: P2. Rework draining in MediaFormatReader.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1337559: P3. Fix coding style of windows decoder.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1337559: [WMF] P4. Only drain one frame at a time.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1337559: P5. Only continue drain once all pending frames are processed.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Currently, when draining, all frames returned by the decoder are first converted from NV12 to BGRA32 and then put into an array and then returned. With the windows decoder, the latency is high and it may have buffered over 30 frames. The makes draining a memory intensive operation (say 30 frames @ 4K, that's close to 1GB of memory) We should instead drain the frames one at a time and use the new mechanism provided in bug 1336947.
OS: Unspecified → Windows
Comment on attachment 8836257 [details] Bug 1337559: [WMF] P1. Avoid draining the audio decoder twice in a row. https://reviewboard.mozilla.org/r/111738/#review113074
Attachment #8836257 - Flags: review?(gsquelart) → review+
Comment on attachment 8836258 [details] Bug 1337559: P2. Rework draining in MediaFormatReader. https://reviewboard.mozilla.org/r/111740/#review113076 r+ with suggestions & questions: ::: dom/media/MediaFormatReader.h:277 (Diff revision 1) > MozPromiseHolder<ShutdownPromise> mShutdownPromise; > MozPromiseRequestHolder<ShutdownPromise> mShutdownRequest; > > bool HasPendingDrain() const > { > - return mDraining || mDrainComplete; > + return mDrainState >= DrainState::DrainRequested; You're using the order of enums, it feels a bit dangerous! At the minimum, you may want to mention that the order is important in the enum itself around line 167. Alternatively, this test could just be `mDrainState != DrainState::None`. ::: dom/media/MediaFormatReader.h:397 (Diff revision 1) > mLastSampleTime.reset(); > mOutput.Clear(); > mNumSamplesInput = 0; > mNumSamplesOutput = 0; > mSizeOfQueue = 0; > + mDrainState = DrainState::None; I think this line should be right after `mDrainRequest.DisconnectIfExists();` -- They're related, and it's also in the same order in the member declaration (and therefore in memory). ::: dom/media/MediaFormatReader.cpp:1776 (Diff revision 1) > - decoder.mNeedDraining = true; > + MOZ_RELEASE_ASSERT(decoder.mDrainState > + == DecoderData::DrainState::None); > + decoder.mDrainState = DecoderData::DrainState::DrainRequested; Third time I see this code; Would it be worth refactoring it into a method? ::: dom/media/MediaFormatReader.cpp:1887 (Diff revision 1) > MediaFormatReader::DrainDecoder(TrackType aTrack) > { > MOZ_ASSERT(OnTaskQueue()); > > auto& decoder = GetDecoderData(aTrack); > - if (!decoder.mNeedDraining || decoder.mDraining) { > + if (decoder.mDrainState == DecoderData::DrainState::Draining) { I don't see an equivalent of the old `!mNeedDraining`, which would supposedly now be `mDrainState == None`. Is that intended? ::: dom/media/MediaFormatReader.cpp:2036 (Diff revision 1) > } > } else if (decoder.HasFatalError()) { > LOG("Rejecting %s promise: DECODE_ERROR", TrackTypeToStr(aTrack)); > decoder.RejectPromise(decoder.mError.ref(), __func__); > return; > - } else if (decoder.mDrainComplete) { > + } else if (decoder.mDrainState >= DecoderData::DrainState::DrainCompleted) { Another enum-order-dependent test, still scary!
Attachment #8836258 - Flags: review?(gsquelart) → review+
Comment on attachment 8836259 [details] Bug 1337559: P3. Fix coding style of windows decoder. https://reviewboard.mozilla.org/r/111742/#review113080 r+ with nits: ::: dom/media/platforms/wmf/DXVA2Manager.cpp:133 (Diff revision 1) > > void GetDXVA2ExtendedFormatFromMFMediaType(IMFMediaType *pType, > DXVA2_ExtendedFormat *pFormat) > { > // Get the interlace mode. > - MFVideoInterlaceMode interlace = > + MFVideoInterlaceMode interlace = (MFVideoInterlaceMode)MFGetAttributeUINT32( While you're here, could you please make the cast C++-style? (There's another one at line 139 below.) ::: dom/media/platforms/wmf/WMFDecoderModule.cpp (Diff revision 1) > - bool hasdecoder = false; > - { > - RefPtr<MFTDecoder> decoder(new MFTDecoder()); > + RefPtr<MFTDecoder> decoder(new MFTDecoder()); > - hasdecoder = SUCCEEDED(decoder->Create(aGuid)); > + bool hasdecoder = SUCCEEDED(decoder->Create(aGuid)); > - } > wmf::MFShutdown(); You've changed the logic here: Previously `decoder` was destroyed at the end of the block, before MFShutdown(). Now you're letting `decoder` live until the end of the function. Are you sure it's ok to have an MFTDecoder still exist while MFShutdown() is being called; and have it destroyed after MFShutdown()? ::: dom/media/platforms/wmf/WMFUtils.h:23 (Diff revision 1) > -inline int64_t > -UsecsToHNs(int64_t aUsecs) { > +inline int64_t UsecsToHNs(int64_t aUsecs) > +{ According to the coding standards, the return type should be on its own line. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods Also, this was not applied consistently anyway, e.g. `HNsToFrames` was left split (as it should be) below, so there's something fishy going on with your stylist! ::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:122 (Diff revision 1) > - (mGotValidOutputAfterNullOutput && mGotExcessiveNullOutput) ? 1 : > - mGotExcessiveNullOutput ? 2 : > - mGotValidOutputAfterNullOutput ? 3 : > - 4; > + (mNullOutputCount == 0) > + ? 0 > + : (mGotValidOutputAfterNullOutput && mGotExcessiveNullOutput) > + ? 1 > + : mGotExcessiveNullOutput ? 2 > + : mGotValidOutputAfterNullOutput ? 3 : 4; Three styles for four ternary operators in one statement! The 3rd one should probably be moved to its own line, similar to the first two (I'm interpreting the coding style that way.)
Attachment #8836259 - Flags: review?(gsquelart) → review+
Comment on attachment 8836260 [details] Bug 1337559: [WMF] P4. Only drain one frame at a time. https://reviewboard.mozilla.org/r/111744/#review113082 r+ with suggestion: ::: dom/media/platforms/wmf/WMFMediaDataDecoder.h:141 (Diff revision 1) > - bool mDrained = true; > + DRAINABLE, > + DRAINING, > + DRAINED, I'd put `DRAINED` as the first state, since it's the default, and probably the most common state across the decoder's lifetime.
Attachment #8836260 - Flags: review?(gsquelart) → review+
Comment on attachment 8836261 [details] Bug 1337559: P5. Only continue drain once all pending frames are processed. https://reviewboard.mozilla.org/r/111746/#review113084 ::: dom/media/MediaFormatReader.cpp:1 (Diff revision 1) > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ Typo in commit description: 'as the come' -> 'as they come'.
Attachment #8836261 - Flags: review?(gsquelart) → review+
Comment on attachment 8836258 [details] Bug 1337559: P2. Rework draining in MediaFormatReader. https://reviewboard.mozilla.org/r/111740/#review113102 ::: dom/media/MediaFormatReader.h:397 (Diff revision 1) > mLastSampleTime.reset(); > mOutput.Clear(); > mNumSamplesInput = 0; > mNumSamplesOutput = 0; > mSizeOfQueue = 0; > + mDrainState = DrainState::None; I made the order the same as in the previous declaration ::: dom/media/MediaFormatReader.cpp:1887 (Diff revision 1) > MediaFormatReader::DrainDecoder(TrackType aTrack) > { > MOZ_ASSERT(OnTaskQueue()); > > auto& decoder = GetDecoderData(aTrack); > - if (!decoder.mNeedDraining || decoder.mDraining) { > + if (decoder.mDrainState == DecoderData::DrainState::Draining) { Yes. because this method is only ever called from a place where mDrainRequested is first tested so i figured there was no point testing it again, and that it should be up to the caller to ensure the callee is called properly ::: dom/media/MediaFormatReader.cpp:2036 (Diff revision 1) > } > } else if (decoder.HasFatalError()) { > LOG("Rejecting %s promise: DECODE_ERROR", TrackTypeToStr(aTrack)); > decoder.RejectPromise(decoder.mError.ref(), __func__); > return; > - } else if (decoder.mDrainComplete) { > + } else if (decoder.mDrainState >= DecoderData::DrainState::DrainCompleted) { Why is that scary?
Comment on attachment 8836259 [details] Bug 1337559: P3. Fix coding style of windows decoder. https://reviewboard.mozilla.org/r/111742/#review113080 > You've changed the logic here: Previously `decoder` was destroyed at the end of the block, before MFShutdown(). > Now you're letting `decoder` live until the end of the function. > Are you sure it's ok to have an MFTDecoder still exist while MFShutdown() is being called; and have it destroyed after MFShutdown()? Oh good point. totally missed thst > According to the coding standards, the return type should be on its own line. > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods > > Also, this was not applied consistently anyway, e.g. `HNsToFrames` was left split (as it should be) below, so there's something fishy going on with your stylist! I have seen plenty of places where the return type is one the same line when its used within a class header its never on in its own line then. being an inline, private variable of the file, it felt logical to me to hse the same principal as an inner declarstion of a class. except that here it's a file rsther than a class > Three styles for four ternary operators in one statement! > The 3rd one should probably be moved to its own line, similar to the first two (I'm interpreting the coding style that way.) The coding style about operator is only about when such operand/operetor wouldn't fill the 80 columns. It does here
Comment on attachment 8836258 [details] Bug 1337559: P2. Rework draining in MediaFormatReader. https://reviewboard.mozilla.org/r/111740/#review113102 > I made the order the same as in the previous declaration When I look at the diff, the removed mNeedDraining was at line 385, mDraining 388, mDrainComplete 389, and you added mDrainState (which replaces the previous three) below at line 397. But if you look at its class-member declaration, it's at line 268, where mDraining was, and just below mDrainRequest. That's why I was suggesting to have `mDrainState = ...` where `mDraining = ...` was, just below `mDrainRequest.DisconnectIfExists();`. > I made the order the same as in the previous declaration I guess you mean the previous "statement" at line 345 in Flush(). Since you're updating this file, I would suggest you also modify Flush() to be consistent with the class member declaration order. ;-) > Yes. because this method is only ever called from a place where mDrainRequested is first tested > > so i figured there was no point testing it again, and that it should be up to the caller to ensure the callee is called properly Makes sense, thank you. > Why is that scary? As I said in the similar issue at line 277, you are relying on the order of the enums to always stay the same, which seems a bit dangerous to me. As above, I would recommend that you at least add a mention in the enum, that the order is important and shouldn't be changed lightly. Or better (I think), you could explicitly test for all the values you care about, in this case DrainCompleted and DrainAborted.
Comment on attachment 8836259 [details] Bug 1337559: P3. Fix coding style of windows decoder. https://reviewboard.mozilla.org/r/111742/#review113080 > The coding style about operator is only about when such operand/operetor wouldn't fill the 80 columns. It does here I would argue that the whole sub-expression 'a ? b : c' does not fit in 80 columns, and therefore should be split on three lines: a ? b : c Two lines seem just weird to me: a ? b : c But it's ambiguous in the coding style page, so I'm not sure if I'm correct here. Not a big deal in the end, at least there is some alignment happening.
Comment on attachment 8836258 [details] Bug 1337559: P2. Rework draining in MediaFormatReader. https://reviewboard.mozilla.org/r/111740/#review113120 ::: dom/media/MediaFormatReader.h:277 (Diff revision 1) > MozPromiseHolder<ShutdownPromise> mShutdownPromise; > MozPromiseRequestHolder<ShutdownPromise> mShutdownRequest; > > bool HasPendingDrain() const > { > - return mDraining || mDrainComplete; > + return mDrainState >= DrainState::DrainRequested; ok.. But the order of state is important. After all you progress from one state to the next. ::: dom/media/MediaFormatReader.cpp:2036 (Diff revision 1) > } > } else if (decoder.HasFatalError()) { > LOG("Rejecting %s promise: DECODE_ERROR", TrackTypeToStr(aTrack)); > decoder.RejectPromise(decoder.mError.ref(), __func__); > return; > - } else if (decoder.mDrainComplete) { > + } else if (decoder.mDrainState >= DecoderData::DrainState::DrainCompleted) { ok.. will manually test the state value
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ee63b4d05662 [WMF] P1. Avoid draining the audio decoder twice in a row. r=gerald https://hg.mozilla.org/integration/autoland/rev/c46c63541168 P2. Rework draining in MediaFormatReader. r=gerald https://hg.mozilla.org/integration/autoland/rev/236e090024cb P3. Fix coding style of windows decoder. r=gerald https://hg.mozilla.org/integration/autoland/rev/f45a1fe5dbfd [WMF] P4. Only drain one frame at a time. r=gerald https://hg.mozilla.org/integration/autoland/rev/ddef25734a30 P5. Only continue drain once all pending frames are processed. r=gerald
Depends on: 1340956
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: