Filed by: philringnalda [at] gmail.com https://treeherder.mozilla.org/logviewer.html#?job_id=11298599&repo=fx-team https://archive.mozilla.org/pub/firefox/tinderbox-builds/fx-team-linux-debug/1472175378/fx-team_ubuntu32_vm-debug_test-mochitest-media-bm08-tests1-linux32-build160.txt.gz

Comment on attachment 8787089 [details] Bug 1298305 - Skip the beginning of null frames. https://reviewboard.mozilla.org/r/75950/#review73966 What does mFirstFrame mean? No doc and I failed to infer from code and name.

Comment on attachment 8787089 [details] Bug 1298305 - Skip the beginning of null frames. https://reviewboard.mozilla.org/r/75950/#review74360 ::: dom/media/encoder/TrackEncoder.h:262 (Diff revision 2) > , mTrackRate(aTrackRate) > , mTotalFrameDuration(0) > , mLastFrameDuration(0) > , mVideoBitrate(0) > , mLastFrameTimeStamp(TimeStamp::Now()) > - , mFirstFrame(true) > + , mFirstNonNullFrameAppended(true) I think the name is fine but the bug is here. It should be false at construction. I also think we don't need it at all. More on that below. ::: dom/media/encoder/TrackEncoder.h:365 (Diff revision 2) > - bool mFirstFrame; > + // This is a temporal solution for migrating from duration-based to > + // timestamp-based VideoTrackEncoder. Sp we need to know the first > + // non-null frame for calculating the duration between frames. > + bool mFirstNonNullFrameAppended; Let's remove this and use mLastFrameTimeStamp as the source of truth instead. ::: dom/media/encoder/TrackEncoder.cpp:288 (Diff revision 2) > + > + RefPtr<layers::Image> image = chunk.mFrame.GetImage(); > + // The video segment might contain empty frame in the beginning. > + // Skip those frame. > + if (!mFirstNonNullFrameAppended && !image) { > + continue; > + } > + Not needed. ::: dom/media/encoder/TrackEncoder.cpp:305 (Diff revision 2) > - RefPtr<layers::Image> image = chunk.mFrame.GetImage(); > > // Fixme: see bug 1290777. We should remove the useage of duration here and > // in |GetEncodedTrack|. > StreamTime duration; > - if (mFirstFrame) > + if (mFirstNonNullFrameAppended) Make this `if (!mLastFrameTimeStamp)` instead. ::: dom/media/encoder/TrackEncoder.cpp:321 (Diff revision 2) > if (image) { > mRawSegment.AppendFrame(image.forget(), > duration, > chunk.mFrame.GetIntrinsicSize(), > PRINCIPAL_HANDLE_NONE, > chunk.mFrame.GetForceBlack()); > mLastFrameDuration = 0; > } > mLastFrameTimeStamp = chunk.mTimeStamp; Move the assignment of mLastFrameTimeStamp into the `if (image)` block. Hopefully that doesn't break anything. If it does we should probably fix that instead.

Comment on attachment 8787089 [details] Bug 1298305 - Skip the beginning of null frames. https://reviewboard.mozilla.org/r/75950/#review74854 ::: dom/media/encoder/TrackEncoder.cpp:283 (Diff revisions 2 - 3) > ReentrantMonitorAutoEnter mon(mReentrantMonitor); > > // Append all video segments from MediaStreamGraph, including null an > // non-null frames. > VideoSegment::ChunkIterator iter(const_cast<VideoSegment&>(aSegment)); > while (!iter.IsEnded()) { Let's make this a for loop. Otherwise your guard with `continue` is broken at the moment. ::: dom/media/encoder/TrackEncoder.cpp:291 (Diff revisions 2 - 3) > - if (!mFirstNonNullFrameAppended && !image) { > + if (mLastFrameTimeStamp.IsNull() && !image) { > continue; > } Not needed. I think you need to pull the mLastFrame assignment at the bottom of the loop in to the `if (image)` block as well, but I can't see any side effects. ::: dom/media/encoder/TrackEncoder.cpp:304 (Diff revisions 2 - 3) > (mLastFrameDuration >= mTrackRate)) { > > // Fixme: see bug 1290777. We should remove the useage of duration here and > // in |GetEncodedTrack|. > StreamTime duration; > - if (mFirstNonNullFrameAppended) > + if (mLastFrameTimeStamp.IsNull()) This can be `if (!mLastFrameTimeStamp)`.

Comment on attachment 8787089 [details] Bug 1298305 - Skip the beginning of null frames. https://reviewboard.mozilla.org/r/75950/#review74998 ::: dom/media/encoder/TrackEncoder.cpp:282 (Diff revisions 3 - 4) > VideoSegment::ChunkIterator iter(const_cast<VideoSegment&>(aSegment)); > - while (!iter.IsEnded()) { > + for (; !iter.IsEnded();iter.Next()) { Nit: The iterator could be declared in the for as well, then we don't keep it around outside the for loop's scope.

The solution of 1300871 might also fix this bug. Let's monitor it.

Comment on attachment 8787089 [details] Bug 1298305 - Skip the beginning of null frames. https://reviewboard.mozilla.org/r/75950/#review87820 bug was duped