Webrtc mediaRecorder.start timeslice not working

RESOLVED FIXED in Firefox 60

Status

()

P2
normal
Rank:
19
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: dankharadivyesh, Assigned: john357smith)

Tracking

(Depends on: 2 bugs)

56 Branch
mozilla60
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 affected, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

Details

Attachments

(3 attachments, 11 obsolete attachments)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

Just start Mediarecorder with timeslice


Actual results:

ondataavailable event is called by interval of timeslice. but all time event.data size is zero only one event per some second has big data like 2.0 MB.
So here timeslice is useless

mediaRecorder.ondataavailable = handleDataAvailable;
mediaRecorder.start(1000); 

function handleDataAvailable(event) {
    console.log('data avail -> ',event.data.size);
}

See Five second dataavailable
data avail -> 312
data avail -> 0
data avail -> 0
data avail -> 0
data avail -> 0
data avail -> 212212


Expected results:

need to work timeslice proper with mediarecorder.start
example
mediaRecorder.ondataavailable = handleDataAvailable;
mediaRecorder.start(1000); 

function handleDataAvailable(event) {
    console.log('data avail -> ',event.data.size);
}

See Five second dataavailable
data avail -> 312
data avail -> 12457
data avail -> 25412
data avail -> 10023
data avail -> 10111
data avail -> 19845
I can repro: https://jsfiddle.net/jib1/73u0ekbz/

Looks like we're not following the intent of the spec here. Looks like we're following some internal interval instead. Open to a fix here if it is trivial.
Has STR: --- → yes
Rank: 19
OS: Unspecified → All
Priority: -- → P2
Status: UNCONFIRMED → NEW
status-firefox56: --- → affected
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox-esr52: --- → affected
Ever confirmed: true
Component: WebRTC → Audio/Video: Recording
I tracked this to EbmlComposer. For VP8 it will only finish a cluster (and write it to the blob) once a keyframe appears, see [1]. We encode a keyframe every 60 frames, [2]. There appears to be a fallback at ~33 seconds if you don't have VP8, [3], (so audio in webm, but we only support audio in ogg, [4]).

WebM spec says, on [5]:
> - Key frames SHOULD be placed at the beginning of clusters.
>   - Having key frames at the beginning of clusters should make seeking faster and easier for the client.

So the question here is whether we should go against this SHOULD (and what it means, we'd still have keyframes at the beginning of clusters, but not all clusters would have keyframes), encode more keyframes, set keyframe interval based on the timeslice, or something else.


[1] http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/dom/media/webm/EbmlComposer.cpp#131
[2] http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/dom/media/encoder/VP8TrackEncoder.cpp#212
[3] http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/dom/media/webm/EbmlComposer.cpp#137
[4] http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/dom/media/MediaRecorder.cpp#1487
[5] https://www.webmproject.org/docs/container/
Another, perhaps simpler, option is to let a blob contain half-finished clusters, but that might take some refactoring of EbmlComposer.
(Assignee)

Comment 4

a year ago
I have been playing with this problem for couple of days so here is my few notes:

- as mentioned above, config.kf_max_dist is set to 60 frames so that means that for aprox. 15 frames per second (which is rate common for most low/middle cost webcams) you get AV data every 3-4 seconds. With lower frames it's getting longer.
- the most ideal solution would be to rewrite whole logic in EbmlComposer so instead of waiting for cluster to be finished just flush out all data and conntinue - currently there is internal buffer used depending on the unfinished cluster data so you can't here just get data, clean buffer and wait for another data - it must be precisely rewritten
- keyframes at the beginning of the cluster are must - I have been facing such a problem a year ago when I tried to find out why is webm/vp8 stream not playing via MSE(can't remember if it was problem of Firefox or Chrome) but fixing stream to have keyframes at the beginning of the cluster was the correct solution

So currently the simplest solution for me is a small change to calculate and set a fixed keyframe interval according the timeslice given to MediaRecorder so cluster would be smaller than actual 60 frames.
 
I already tried to make a patch - changes are attached. Code is taken from firefox-57.0b10 release I tried to compile it did a few tests and it works without any problem. The main change is in VP8TrackEncoder where is logic of counting the best keyframe interval according the track rate, timeslice and frame duration. Frame duration is taken from the first frame so before the data are passed to the encoder the keyframe interval is counted and set. Changes in other modules are only to hand over the timeslice value (MediaRecorder -> MediaEncoder -> VP8TrackEncoder). If a timeslice/flushtime is not set (this is done only from MediaRecoder) a keyframe interval is kept as it is now (60 frames).

Could somebody please review this patch and let me know if it is suitable to move forward?

Thanks.
(Assignee)

Comment 5

a year ago
Posted patch MediaEncoder.cpp.patch (obsolete) — Splinter Review
Attachment #8923196 - Flags: feedback+
(Assignee)

Comment 6

a year ago
Posted patch MediaEncoder.h.patch (obsolete) — Splinter Review
Attachment #8923197 - Flags: feedback+
(Assignee)

Comment 7

a year ago
Posted patch MediaRecorder.cpp.patch (obsolete) — Splinter Review
Attachment #8923198 - Flags: feedback+
(Assignee)

Comment 8

a year ago
Posted patch TrackEncoder.h.patch (obsolete) — Splinter Review
Attachment #8923199 - Flags: feedback+
(Assignee)

Comment 9

a year ago
Posted patch VP8TrackEncoder.cpp.patch (obsolete) — Splinter Review
Attachment #8923200 - Flags: feedback+
(Assignee)

Comment 10

a year ago
Posted patch VP8TrackEncoder.h.patch (obsolete) — Splinter Review
Attachment #8923201 - Flags: feedback+
I think John meant to mark these feedback? not feedback+
Flags: needinfo?(bvandyk)
(Assignee)

Comment 12

a year ago
Yes sorry, I didn't know what's different between feedback+ and feedback.

Thanks.
Various thoughts:

The timeslice is a lower bound on firing a blob based on my reading of the spec. I think it's intuitive to fire at the rate specified by timeslice, but there is some wiggle room here (it is possible for the UA to set a minimum timeslice which the arg cannot go under)[1].

I do not believe Chrome set their own bound, and will fire an event if the passed time has exceeded the user set timeslice[2]. Looks like the call chain there is VideoTrackRecorder::Encoder::OnFrameEncodeCompleted -> MediaRecorderHandler::OnEncodedVideo -> WebmMuxer::OnEncodedVideo which then starts to call into libwebm[3]. MediaRecorderHandler::WriteData gets called back by libwebm as part of its data writing callback. I believe libwebms muxer will write on block boundries.

Agreed that keyframes at the start of clusters are a must.

Agreed we can do better here. What use cases are we interested in? The most compelling use case for getting data available as soon as possible is for Media Source Extensions or Real Time Communication (though her perhaps we'd send the input to RTC rather than the recorder output data?). In the MSE case I think we can output non finished clusters that can be appended to buffers (like libwebm we could write out on the block scale).

We could also tweak the existing keyframe rate so that we continue to write out on cluster boundaries, but are more flexible. I'm concerned as to the results of this for small timeslices.

So 2 major solutions: write on blocks, not clusters, or adjust keyframe rate. For the first we could either rework EBMLComposer, or we could replace the existing machinery with libwebm. The former is likely easier, though I have an old branch that uses libwebm (will see if I can dredge it up and if it rebases).

Interested in other people's thoughts.

[1]: https://w3c.github.io/mediacapture-record/MediaRecorder.html#mediarecorder-methods
[2]: https://cs.chromium.org/chromium/src/content/renderer/media_recorder/media_recorder_handler.cc?sq=package:chromium&l=406
[3]: https://github.com/webmproject/libwebm
Flags: needinfo?(bvandyk)
"When multiple Blobs are returned (because of timeslice or requestData()), the individual Blobs need not be playable, but the combination of all the Blobs from a completed recording MUST be playable."

So given that, why do blobs need to start with keyframes?
(In reply to Randell Jesup [:jesup] from comment #14)
> "When multiple Blobs are returned (because of timeslice or requestData()),
> the individual Blobs need not be playable, but the combination of all the
> Blobs from a completed recording MUST be playable."
> 
> So given that, why do blobs need to start with keyframes?

They don't, it's just our code's legacy. It would be some work to write to the blob when we finish a block rather than a cluster. FWIW I think this refactor (write on clusters -> blocks) would be the cleanest. Bryce, do you have an idea of how much work that would be?
Flags: needinfo?(bvandyk)
(In reply to Randell Jesup [:jesup] from comment #14)
> "When multiple Blobs are returned (because of timeslice or requestData()),
> the individual Blobs need not be playable, but the combination of all the
> Blobs from a completed recording MUST be playable."
> 
> So given that, why do blobs need to start with keyframes?

It's okay if the blobs don't start we keyframes. However, if once the blobs are assembled they constitute a webm where a cluster starts with a non-key frame then that's not desirable as demuxers may rely on clusters starting with keyframes (sensibly IMO given the spec).

> Bryce, do you have an idea of how much work that would be?

I'll take a look. Off the top of my head I'd estimate a week or two, but it's been awhile since I was dug into the code. Am about to relocate, but am hoping to be largely back online at the start of next week and will revise estimate as needed then.
Flags: needinfo?(bvandyk)
Most of the work to get us onto the new backend is hopefully done. However, I remember that some of this work requires us to do more rigorous syncing within the muxer. Right now we'll happily pass frames with out of order time stamps to the muxer, which works fine with our libmkv based backend (though these will likely be discarded by various demuxers), but the libwebm based backend will reject these frames.

I've got some old WIP on Bug 1014393 to do with better handling around this behaviour. Going to rebase and get that done to enable these changes.
Depends on: 1014393
(Assignee)

Comment 21

a year ago
What is the progress with this bug? For what version can we expect a fix?

Thanks.
Flags: needinfo?(jib)
I'm currently working on a couple of other bugs that I resolving before this. I'd hope for a 60 or 61 release for this fix.
(Assignee)

Comment 23

a year ago
If I see it correctly a Release 60 is planned on May, Release 61 on July which is quite a long time. Would it be possible to merge my proposed patches as a temporary solution?

Thanks.
(In reply to john357smith from comment #23)
> If I see it correctly a Release 60 is planned on May, Release 61 on July
> which is quite a long time.

That's right, but to make sure there's no misunderstanding, the earliest release current changes will ship in is 59 (the current version of nightly).

> Would it be possible to merge my proposed patches as a temporary solution?

That sounds like it could be a good way to address this in the meantime. I'll move this up in me queue.
(Assignee)

Comment 25

a year ago
OK, thank you.
Comment on attachment 8923196 [details] [diff] [review]
MediaEncoder.cpp.patch

Re comment 12. Did this get looked on? If so, please set flags appropriately.
Flags: needinfo?(jib)
Attachment #8923196 - Flags: feedback+ → feedback?(bvandyk)
Attachment #8923197 - Flags: feedback+ → feedback?(bvandyk)
Attachment #8923198 - Flags: feedback+ → feedback?(bvandyk)
Attachment #8923199 - Flags: feedback+ → feedback?(bvandyk)
Attachment #8923200 - Flags: feedback+ → feedback?(bvandyk)
Attachment #8923201 - Flags: feedback+ → feedback?(bvandyk)
Comment on attachment 8923196 [details] [diff] [review]
MediaEncoder.cpp.patch

Review of attachment 8923196 [details] [diff] [review]:
-----------------------------------------------------------------

::: ../firefox-57.0b10/dom/media/encoder/MediaEncoder_orig.cpp
@@ +429,5 @@
>  
> +void
> +MediaEncoder::SetFlushTime(int32_t aFlushTime)
> +{
> +  mVideoEncoder->SetFlushTime(aFlushTime);

This needs a check to see if mVideoEncoder exists, otherwise it may attempt to deref on a null RefPtr (audio only recorders).
Comment on attachment 8923197 [details] [diff] [review]
MediaEncoder.h.patch

Review of attachment 8923197 [details] [diff] [review]:
-----------------------------------------------------------------

::: ../firefox-57.0b10/dom/media/encoder/MediaEncoder_orig.h
@@ +220,4 @@
>      return mVideoSink.get();
>    }
>  
> +  // Set desired flush time for encoded data

Could we name this more explicitly to refer to setting the keyframe interval: something like SetVideoKeyframeInterval. This could help avoid people mistaking the function as interacting with audio flushing.
Attachment #8923198 - Flags: feedback?(bvandyk) → feedback+
Comment on attachment 8923199 [details] [diff] [review]
TrackEncoder.h.patch

Review of attachment 8923199 [details] [diff] [review]:
-----------------------------------------------------------------

::: ../firefox-57.0b10/dom/media/encoder/TrackEncoder_orig.h
@@ +386,5 @@
>  
>    uint32_t mVideoBitrate;
> +
> +  // flush time for encoded data
> +  int32_t mFlushTime;

As with some of the other naming, I wonder if we could have this more explicitly refer to video key frames to avoid ambiguity with audio flushing.
Comment on attachment 8923201 [details] [diff] [review]
VP8TrackEncoder.h.patch

Review of attachment 8923201 [details] [diff] [review]:
-----------------------------------------------------------------

::: ../firefox-57.0b10/dom/media/encoder/VP8TrackEncoder_orig.h
@@ +56,5 @@
>    nsresult Reconfigure(int32_t aWidth, int32_t aHeight,
>                         int32_t aDisplayWidth, int32_t aDisplayHeight);
>  
> +  // Only re-configures an existing encoder
> +  nsresult ResetOnlyConfiguration();

Could we call this something like 'ReconfigurePreserveDimensions' or 'ReconfigurePreserveResolution'? This is a stripped down version of Reconfigure, so would be good to make it clear that it's doing a similar thing.
Comment on attachment 8923200 [details] [diff] [review]
VP8TrackEncoder.cpp.patch

Review of attachment 8923200 [details] [diff] [review]:
-----------------------------------------------------------------

::: ../firefox-57.0b10/dom/media/encoder/VP8TrackEncoder_orig.cpp
@@ +362,5 @@
>  
> +  // Calculate the best keyframe according the given flush interval
> +  if (mEncodedTimestamp == 0 && mFlushTime > 0) {
> +    mKeyframeInterval = std::max(std::min((int)floor((float)mFlushTime / ((1.0 / mTrackRate) * (unsigned long)aChunk.GetDuration() * 1000)), MAX_KEYFRAME_INTERVAL), 0);
> +    VP8LOG(LogLevel::Info, "A fixed keyframe interval %d was set (track rate: %d, frame duration: %ld, flush time: %d).", mKeyframeInterval, mTrackRate, (unsigned long)aChunk.GetDuration(), mFlushTime);

Could you break these lines to be <= 80 chars?

Could the casts be changed to use explicitly sized types here as per the rest of the file?

I also wonder if we can rework the calculations to not require so many casts.
Feedback above. I'm still testing the patches locally. I'm having some issues with the test case jib provided, and want to take some more time to look at what's going on. john357smith, have you seen the desired results with your patches when using jibs test case?
(Assignee)

Comment 33

a year ago
I will look at your comments but what test case do you mean (what jib's comment)? Sorry I don't see any test case from jib. Thanks.
Flags: needinfo?(bvandyk)
This one here: https://jsfiddle.net/jib1/73u0ekbz/
Flags: needinfo?(bvandyk)
(Assignee)

Comment 35

a year ago
Posted video recording.webm
(Assignee)

Comment 36

a year ago
Posted file recording.log
(Assignee)

Comment 37

a year ago
A tried to run a test code from jib - for me it works. Attached is recorded .webm file (just recorded my second lcd with test pattern running) and output lines from test window.
(Assignee)

Comment 38

a year ago
(In reply to john357smith from comment #37)
> A tried to run a test code from jib - for me it works. Attached is recorded
> .webm file (just recorded my second lcd with test pattern running) and
> output lines from test window.

And just one note - I had to change a constraints a little bit (adding max width and height) because under Linux my cam is working only with max res. 320x240.
Thanks for your reply. I was seeing some issues, but it appears to have been specific to the particular checkout I had for Firefox. After a rebase I'm seeing similar results.
(Assignee)

Comment 40

a year ago
Comment on attachment 8923196 [details] [diff] [review]
MediaEncoder.cpp.patch

--- ../firefox-57.0b10/dom/media/encoder/MediaEncoder_orig.cpp	2017-09-15 06:15:43.000000000 +0200
+++ ../firefox-57.0b10/dom/media/encoder/MediaEncoder.cpp	2017-12-21 16:03:29.000000000 +0100
@@ -427,4 +427,12 @@
   return amount;
 }
 
+void
+MediaEncoder::SetVideoFlushTime(int32_t aVideoFlushTime)
+{
+  if (mVideoEncoder) {
+    mVideoEncoder->SetVideoFlushTime(aVideoFlushTime);
+  }
+}
+
 } // namespace mozilla
(Assignee)

Comment 41

a year ago
Comment on attachment 8923197 [details] [diff] [review]
MediaEncoder.h.patch

--- ../firefox-57.0b10/dom/media/encoder/MediaEncoder_orig.h	2017-06-12 18:37:20.000000000 +0200
+++ ../firefox-57.0b10/dom/media/encoder/MediaEncoder.h	2017-12-21 15:21:50.000000000 +0100
@@ -220,6 +220,9 @@
     return mVideoSink.get();
   }
 
+  // Set desired flush time to compute a correct video keyframe interval
+  void SetVideoFlushTime(int32_t aVideoFlushTime);
+
 private:
   // Get encoded data from trackEncoder and write to muxer
   nsresult WriteEncodedDataToMuxer(TrackEncoder *aTrackEncoder);
(Assignee)

Comment 42

a year ago
Comment on attachment 8923198 [details] [diff] [review]
MediaRecorder.cpp.patch

--- ../firefox-57.0b10/dom/media/MediaRecorder_orig.cpp	2017-09-15 06:15:42.000000000 +0200
+++ ../firefox-57.0b10/dom/media/MediaRecorder.cpp	2017-12-21 15:21:44.000000000 +0100
@@ -806,6 +806,10 @@
       LOG(LogLevel::Debug, ("Session.InitEncoder !ReadThread->Dispatch %p", this));
       DoSessionEndTask(NS_ERROR_ABORT);
     }
+
+    // Set desired flush time to compute a correct video keyframe interval
+    mEncoder->SetVideoFlushTime(mTimeSlice);
+
     // Set mNeedSessionEndTask to false because the
     // ExtractRunnable/DestroyRunnable will take the response to
     // end the session.
Attachment #8923198 - Flags: feedback+ → feedback?(bvandyk)
(Assignee)

Comment 43

a year ago
Comment on attachment 8923199 [details] [diff] [review]
TrackEncoder.h.patch

--- ../firefox-57.0b10/dom/media/encoder/TrackEncoder_orig.h	2017-10-28 21:05:35.000000000 +0200
+++ ../firefox-57.0b10/dom/media/encoder/TrackEncoder.h	2017-12-21 16:22:46.000000000 +0100
@@ -272,6 +272,7 @@
     , mTrackRate(aTrackRate)
     , mEncodedTicks(0)
     , mVideoBitrate(0)
+    , mVideoFlushTime(-1)
   {
     mLastChunk.mDuration = 0;
   }
@@ -305,6 +306,12 @@
     return mTrackRate * aS;
   }
 
+  // Set desired flush time to compute a correct video keyframe interval
+  void SetVideoFlushTime(float aVideoFlushTime)
+  {
+    mVideoFlushTime = aVideoFlushTime;
+  }
+
 protected:
   /**
    * Initialized the video encoder. In order to collect the value of width and
@@ -378,6 +385,9 @@
   TimeStamp mStartOffset;
 
   uint32_t mVideoBitrate;
+
+  // flush time to compute a correct video keyframe interval
+  float mVideoFlushTime;
 };
 
 } // namespace mozilla
(Assignee)

Comment 44

a year ago
Comment on attachment 8923200 [details] [diff] [review]
VP8TrackEncoder.cpp.patch

--- ../firefox-57.0b10/dom/media/encoder/VP8TrackEncoder_orig.cpp	2017-10-28 14:43:47.000000000 +0200
+++ ../firefox-57.0b10/dom/media/encoder/VP8TrackEncoder.cpp	2017-12-21 16:24:01.000000000 +0100
@@ -26,6 +26,7 @@
                                         (msg, ##__VA_ARGS__))
 
 #define DEFAULT_BITRATE_BPS 2500000
+#define MAX_KEYFRAME_INTERVAL 60
 
 using namespace mozilla::gfx;
 using namespace mozilla::layers;
@@ -59,6 +60,7 @@
 VP8TrackEncoder::VP8TrackEncoder(TrackRate aTrackRate)
   : VideoTrackEncoder(aTrackRate)
   , mEncodedTimestamp(0)
+  , mKeyframeInterval(-1)
   , mVPXContext(new vpx_codec_ctx_t())
   , mVPXImageWrapper(new vpx_image_t())
 {
@@ -160,6 +162,23 @@
 }
 
 nsresult
+VP8TrackEncoder::ReconfigurePreserveResolution()
+{
+  // Encoder configuration structure.
+  vpx_codec_enc_cfg_t config;
+  nsresult rv = SetConfigurationValues(mFrameWidth, mFrameHeight, mDisplayWidth, mDisplayHeight, config);
+  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
+
+  // Set new configuration
+  if (vpx_codec_enc_config_set(mVPXContext.get(), &config) != VPX_CODEC_OK) {
+    VP8LOG(LogLevel::Error, "Failed to set new configuration");
+    return NS_ERROR_FAILURE;
+  }
+
+  return NS_OK;
+}
+
+nsresult
 VP8TrackEncoder::SetConfigurationValues(int32_t aWidth, int32_t aHeight, int32_t aDisplayWidth,
                                         int32_t aDisplayHeight, vpx_codec_enc_cfg_t& config)
 {
@@ -214,8 +233,14 @@
   config.rc_buf_sz = 1000;
 
   config.kf_mode = VPX_KF_AUTO;
-  // Ensure that we can output one I-frame per second.
-  config.kf_max_dist = 60;
+
+  // If there is a defined keyframe interval use it otherwise keep the previous default value
+  if (mKeyframeInterval >= 0) {
+    config.kf_min_dist = mKeyframeInterval;
+    config.kf_max_dist = mKeyframeInterval;
+  } else {
+    config.kf_max_dist = MAX_KEYFRAME_INTERVAL;
+  }
 
   return NS_OK;
 }
@@ -335,6 +360,16 @@
     img = aChunk.mFrame.GetImage();
   }
 
+  // Calculate the best keyframe according the given video flush interval
+  if (mEncodedTimestamp == 0 && mVideoFlushTime > 0) {
+    mKeyframeInterval = std::max(std::min(
+      (int)floor(mVideoFlushTime / ((1.0 / mTrackRate) * aChunk.GetDuration() * 1000)),
+      MAX_KEYFRAME_INTERVAL), 0);
+    VP8LOG(LogLevel::Info,
+      "A fixed keyframe interval %d was set (track rate: %d, frame duration: %ld, flush time: %f).",
+      mKeyframeInterval, mTrackRate, aChunk.GetDuration(), mVideoFlushTime);
+  }
+
   if (img->GetSize() != IntSize(mFrameWidth, mFrameHeight)) {
     VP8LOG(LogLevel::Info,
            "Dynamic resolution change (was %dx%d, now %dx%d).",
@@ -361,6 +396,10 @@
       VP8LOG(LogLevel::Info, "Recreated VP8 encoder.");
       NS_ENSURE_SUCCESS(rv, rv);
     }
+  } else if (mEncodedTimestamp == 0) {
+    // Frame is not different so update only configuration
+    nsresult rv = ReconfigurePreserveResolution();
+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
   }
 
   ImageFormat format = img->GetFormat();
(Assignee)

Comment 45

a year ago
Comment on attachment 8923201 [details] [diff] [review]
VP8TrackEncoder.h.patch

--- ../firefox-57.0b10/dom/media/encoder/VP8TrackEncoder_orig.h	2017-06-12 18:37:20.000000000 +0200
+++ ../firefox-57.0b10/dom/media/encoder/VP8TrackEncoder.h	2017-12-21 16:24:05.000000000 +0100
@@ -56,6 +56,9 @@
   nsresult Reconfigure(int32_t aWidth, int32_t aHeight,
                        int32_t aDisplayWidth, int32_t aDisplayHeight);
 
+  // Only re-configures an existing encoder
+  nsresult ReconfigurePreserveResolution();
+
   // Destroys the context and image wrapper. Does not de-allocate the structs.
   void Destroy();
 
@@ -78,6 +81,9 @@
   // I420 frame, for converting to I420.
   nsTArray<uint8_t> mI420Frame;
 
+  // A keyframe interval
+  int32_t mKeyframeInterval;
+
   /**
    * A local segment queue which takes the raw data out from mRawSegment in the
    * call of GetEncodedTrack().
(Assignee)

Comment 46

a year ago
I'm sorry for commented patches - I supposed this is not correct way to submit a fixed patch? How can I edit a patch without deleting a previous one and still keeping the same name?

Anyway:
- mVideoEncoder check added
- regarding the naming - I replace "FlushTime" with "VideoFlushTime" because I would avoid naming it with "keyframe". It's not setting kefyrame interval instead it's a kind of setting variable to help set a correct keyframe later in the code.
- ResetOnlyConfiguration changed to proposed ReconfigurePreserveResolution()
- a keyframe setting - reformatted and removed useless explicit type casts (but still had to keep one because of min() function 
   - really don't know how to do it somehow else).

Please let me know ... Thanks.
Flags: needinfo?(bvandyk)
Patches should be attached to the bug -- and not usually one-per-file, as the 2-month-old ones are, but usually one patch that resolves an issue (with complex changes, changes are broken into understandable chunks -- again not by file, but by a coherent changeset.  

Then you can mark the patch for review or feedback, and the review tools work.
(Assignee)

Comment 48

a year ago
Posted patch patchset_20171221.patch (obsolete) — Splinter Review
Attachment #8938436 - Flags: feedback?(bvandyk)
(Assignee)

Comment 49

a year ago
OK thanks for clarification - attached one file (one issue) patch file.
Comment on attachment 8938436 [details] [diff] [review]
patchset_20171221.patch

Review of attachment 8938436 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updated patch! Updated feedback attached. Would it be possible to update the patch to be based off central? I had some issues applying the patch locally and this could be an issue if we look at merging once we get to reviewing. I'll attach my locally applied changes shortly, which will hopefully simplify getting these changes onto local centrals.

::: ../firefox-57.0b10/dom/media/encoder/VP8TrackEncoder_orig.cpp
@@ +166,5 @@
> +{
> +  // Encoder configuration structure.
> +  vpx_codec_enc_cfg_t config;
> +  nsresult rv = SetConfigurationValues(mFrameWidth, mFrameHeight, mDisplayWidth, mDisplayHeight, config);
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

Could we use the following instead?
`if (NS_WARN_IF(NS_FAILED(rv))) {
   return NS_ERROR_FAILURE;
 }`

This prevents the return being hidden and is typically preferred in new code (though I appreciate we still have a lot of NS_ENSURES* littering the code).

@@ +363,5 @@
> +  // Calculate the best keyframe according the given video flush interval
> +  if (mEncodedTimestamp == 0 && mVideoFlushTime > 0) {
> +    mKeyframeInterval = std::max(std::min(
> +      (int)floor(mVideoFlushTime / ((1.0 / mTrackRate) * aChunk.GetDuration() * 1000)),
> +      MAX_KEYFRAME_INTERVAL), 0);

I think we can do this without casts similar to:
int32_t keyframeInterval =
  mFlushTime * mTrackRate / (aChunk.GetDuration() * 1000);
mKeyframeInterval =
  std::max(std::min(keyframeInterval, MAX_KEYFRAME_INTERVAL), 0);

@@ +398,5 @@
>      }
> +  } else if (mEncodedTimestamp == 0) {
> +    // Frame is not different so update only configuration
> +    nsresult rv = ReconfigurePreserveResolution();
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

`if (NS_WARN_IF(NS_FAILED(rv))) {
   return NS_ERROR_FAILURE;
 }`

::: ../firefox-57.0b10/dom/media/encoder/VP8TrackEncoder_orig.h
@@ +56,4 @@
>    nsresult Reconfigure(int32_t aWidth, int32_t aHeight,
>                         int32_t aDisplayWidth, int32_t aDisplayHeight);
>  
> +  // Only re-configures an existing encoder

'Reconfigure an existing encoder while preserving the frame size'?

@@ +81,4 @@
>    // I420 frame, for converting to I420.
>    nsTArray<uint8_t> mI420Frame;
>  
> +  // A keyframe interval

Could we describe the units of this member? Something like "A keyframe interval in frames. Set if the encoder needs to output keyframes at a given specific interval."

::: ../firefox-57.0b10/dom/media/encoder/TrackEncoder_orig.h
@@ +385,5 @@
>    TimeStamp mStartOffset;
>  
>    uint32_t mVideoBitrate;
> +
> +  // flush time to compute a correct video keyframe interval

Can we specify the units for this? Something like "the desired time in milliseconds between video keyframes emitted by the encoder". This type has changed from int32_t to float, I think the original int32_t is fine based on this being derived from MediaRecorder's mTimeSlice which is also a int32_t.
My local application of the patch can be found in the try run above, which will also check it against some of our tests. I've also run this through `mach clang-format`, so it will have automated formatting applied.
Flags: needinfo?(bvandyk)
Attachment #8923196 - Flags: feedback?(bvandyk)
Attachment #8923197 - Flags: feedback?(bvandyk)
Attachment #8923198 - Flags: feedback?(bvandyk)
Attachment #8923199 - Attachment is obsolete: true
Attachment #8923199 - Flags: feedback?(bvandyk)
Attachment #8923200 - Attachment is obsolete: true
Attachment #8923200 - Flags: feedback?(bvandyk)
Attachment #8923201 - Attachment is obsolete: true
Attachment #8923201 - Flags: feedback?(bvandyk)
Attachment #8923198 - Attachment is obsolete: true
Attachment #8923197 - Attachment is obsolete: true
Attachment #8923196 - Attachment is obsolete: true
(Assignee)

Comment 53

a year ago
Thanks for a review I will apply your comments. Just question - what do you mean to have patches based off central? I used diff tool to make a patch so should I use something different?
Flags: needinfo?(bvandyk)
By central I mean repository for the main dev tree: https://hg.mozilla.org/mozilla-central

Typically when dealing with patches we use mercurial (hg export) or bzexport: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F We also have some other workflows, such as MozReview. It can be quite involved to setup some of the other workflows, though if you continue to do work they will save you time in the long run. However, feel free to submit patches if that works best.

Any changes to the code will be applied onto the central repository, so it'll make apply those changes that much easier if the patch is based on the tip of that repo.

To make sure we're on the same page, when you're happy with the code being ready to go into the repo you can request review (instead of feedback) and once it's r+ (approved) it can be checked in.

Please let me know if you have any questions about any of the above (or anything)!
Flags: needinfo?(bvandyk)
(Assignee)

Comment 55

a year ago
Thanks for explanation - currently I'm cloning a central repository but it seems it will take quite a long time. I will try to do all proposed above and let you know.

Thanks.
(Assignee)

Comment 56

a year ago
Posted patch patchset_20180107.patch (obsolete) — Splinter Review
Attachment #8940549 - Flags: review?(bvandyk)
(Assignee)

Comment 57

a year ago
I created a new patchset based on central repository and included all your comments. Can I ask for a review?

Thanks.
Comment on attachment 8940549 [details] [diff] [review]
patchset_20180107.patch

Review of attachment 8940549 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, small fix to typo, and could you please provide a commit message on the patch? This can be done by creating a changeset with `hg commit` supplying a message of the format 'Bug 1411857 - <Message describing the changes>' then exporting that changeset/commit. When you upload that patch you can specify that it obsoletes this one and re-request review.

::: dom/media/encoder/VP8TrackEncoder.h
@@ +83,4 @@
>    nsTArray<uint8_t> mI420Frame;
>  
>    /**
> +   * A fixed keyframe interval (iu frames) for video track computed and used

Typo: iu -> in
Attachment #8940549 - Flags: review?(bvandyk)
Attachment #8938436 - Attachment is obsolete: true
Attachment #8938436 - Flags: feedback?(bvandyk)
Comment on attachment 8940549 [details] [diff] [review]
patchset_20180107.patch

Review of attachment 8940549 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for tackling this! I'm in general on board with the approach, but I have some additional comments per below. Most are nits on naming but there are two that have to be fixed before we can land this:
- The race, track encoder methods must be called on the encoder thread
- The key frame interval conversion from milliseconds to number of frames, more details inlined

It would also be great to unit test this. See https://searchfox.org/mozilla-central/source/dom/media/gtest/TestVideoTrackEncoder.cpp
You can run the VP8 encoder tests locally with the command `./mach gtest VP8VideoTrackEncoder*`.

::: dom/media/MediaRecorder.cpp
@@ +981,4 @@
>        mEncoder->ConnectMediaStreamTrack(track);
>      }
>  
> +    // Set desired flush time to compute a correct video keyframe interval

I don't see anything defining "correct video keyframe interval". I'm ok with the approach but let's be honest in the comment with this being a workaround to get timely content for blobs with a short timeslice.

Also, should we call it "SetVideoKeyFrameInterval" instead of "SetVideoFlushTime"? Since it's not all that clear what "flush time" means.

We should IMHO also change `mTimeSlice` and everything depending on it to the TimeDuration type, but that could be a followup. As long as we're clear on the timeslice here being in milliseconds.

::: dom/media/encoder/MediaEncoder.cpp
@@ +1133,5 @@
>  }
>  
> +void
> +MediaEncoder::SetVideoFlushTime(int32_t aVideoFlushTime)
> +{

Add a thread assert. Here it'd be `  MOZ_ASSERT(NS_IsMainThread());`.

@@ +1135,5 @@
> +void
> +MediaEncoder::SetVideoFlushTime(int32_t aVideoFlushTime)
> +{
> +  if (mVideoEncoder) {
> +    mVideoEncoder->SetVideoFlushTime(aVideoFlushTime);

Here you have a race.

It's fine to read `mVideoEncoder` on main thread, but you cannot call into it. Calls into the track encoder need to happen on the worker thread.

So do a dispatch like the many other examples in this file. Since this all grows this method a bit, let's also reduce indentation depth by making the if statement a guard instead:
```
  if (!mVideoEncoder) {
    return;
  }
  MOZ_ASSERT(mEncoderThread);
  nsresult rv =
    mEncoderThread->Dispatch(
      NewRunnableMethod<int32_t>(
        "mozilla::VideoTrackEncoder::SetVideoFlushTime",
        mVideoEncoder, &VideoTrackEncoder::SetVideoFlushTime,
        aVideoFlushTime));
  MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
  Unused << rv;
```

::: dom/media/encoder/MediaEncoder.h
@@ +224,5 @@
>     */
>    size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf);
>  
> +  /**
> +   * Set desired flush time to compute a correct video keyframe interval.

Like my previous comment I think this should drop the "correct" bit. Just say that it sets the video encoder's key frame interval in milliseconds?

::: dom/media/encoder/TrackEncoder.h
@@ +486,5 @@
>     */
>    void AdvanceCurrentTime(StreamTime aDuration) override;
>  
> +  /**
> +   * Set desired flush time to compute a correct video keyframe interval.

And same story for this comment as for the one in MediaEncoder.h. Also you can remove the mentions of video since this is already in the video encoder.

@@ +490,5 @@
> +   * Set desired flush time to compute a correct video keyframe interval.
> +   */
> +  void SetVideoFlushTime(int32_t aVideoFlushTime)
> +  {
> +    mVideoFlushTime = aVideoFlushTime;

You can remove "Video" from these names as this is already in the VideoTrackEncoder.

Also, add a threading assert at the top of this method. It should be `MOZ_ASSERT(!mWorkerThread || mWorkerThread->IsCurrentThreadIn());`.

To do this you move the method definition to the cpp file so we don't have to include the TaskQueue header in this header file (it might still work because of unified builds, but that could break unexpectedly in the future, so let's move it anyhow).

@@ +574,5 @@
>    FrameDroppingMode mFrameDroppingMode;
> +
> +  /**
> +   * The desired flush time in milliseconds between video keyframes emitted by the encoder
> +   * to compute a correct video keyframe interval.

Sounds good with a couple changes I think,
s/flush time/interval/
s/ to compute a correct video keyframe interval//

::: dom/media/encoder/VP8TrackEncoder.cpp
@@ +362,5 @@
>  
> +  // Calculate the best keyframe according the given video flush interval
> +  if (mEncodedTimestamp == 0 && mVideoFlushTime > 0) {
> +    int32_t keyframeInterval = mVideoFlushTime * mTrackRate / (aChunk.GetDuration() * 1000);
> +    mKeyframeInterval = std::min(keyframeInterval, MAX_KEYFRAME_INTERVAL);

This is going to work most of the time but it's not going to cover all cases. Duration of video frames in a video track is not fixed and can fluctuate arbitrarily. Since the most common cases are where sources are webcams, either directly or across a network, it's mostly ok. But we have to cover other cases too.

You'd probably have to continuously look for incoming frames and their durations and update the vp8 encoder's keyframe interval accordingly. Or just leave the kf_max_dist setting and request keyframes manually with the VPX_EFLAG_FORCE_KF flag.

Note though that unless there's a frame coming in you won't be able to emit a key frame, so for very-low-rate video tracks you might still get empty blobs. That's probably a fair corner case to leave out though.

@@ +396,5 @@
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
> +  } else if (mEncodedTimestamp == 0) {
> +    // Frame is not different so update only configuration
> +    nsresult rv = ReconfigurePreserveResolution();

We should trigger this by setting the key frame interval instead of on the first frame.

But as I mentioned in my previous comment we might be better off leaving the configuration alone and handling it manually, in which case this call won't be needed.
(Assignee)

Comment 60

a year ago
Thanks for your comments. Forcing keyframe with VPX_EFLAG_FORCE_KF sounds like a good idea. I will try to make a new patchset according your comments at the weekend.
(Assignee)

Comment 61

a year ago
Posted patch patchset_20180111.patch (obsolete) — Splinter Review
I created a new patchset. Main change is in setting keyframe interval -
instead of previous setting according the first video chunk duration keyframe interval is now forced according the mTimeslice. All your comments (hopefully) included. Can I ask for a review?

Thanks.
Attachment #8940549 - Attachment is obsolete: true
Attachment #8943333 - Flags: review?(apehrson)
Comment on attachment 8943333 [details] [diff] [review]
patchset_20180111.patch

Review of attachment 8943333 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!

Mostly nits -- these I'd normally give r+ with. However, one concern remains on when we reset the non-keyframe duration counter. There are other reasons keyframes get encoded and they need to be accounted for too. More in the inline comment.

::: dom/media/MediaRecorder.cpp
@@ +981,4 @@
>        mEncoder->ConnectMediaStreamTrack(track);
>      }
>  
> +    // Set desired video keyframe interval defined in milliseconds.

I'd still like to expand this comment to cover that we do this to ensure that a short timeslice raises non-empty blobs.

::: dom/media/encoder/TrackEncoder.cpp
@@ +28,4 @@
>  // 30 second threshold if the video encoder cannot be initialized.
>  static const int VIDEO_INIT_FAILED_DURATION = 30;
>  
> +

Superfluous

::: dom/media/encoder/VP8TrackEncoder.cpp
@@ +213,5 @@
> +  // for defined keyframe (>0) interval we force keyframes manually so we can disable automatic keyframe placing
> +  if (mKeyFrameInterval > 0) {
> +    config.kf_mode = VPX_KF_DISABLED;
> +  } else
> +    config.kf_mode = VPX_KF_AUTO;

It's in our style guide to have brackets around single-line else clauses.

@@ +662,5 @@
> +        mNormalFramesDuration += (chunk.GetDuration() * 1000 / mTrackRate);
> +        if (mNormalFramesDuration > mKeyFrameInterval)
> +        {
> +          mNormalFramesDuration = 0;
> +          flags |= VPX_EFLAG_FORCE_KF;

You need to also reset `mNormalFramesDuration` when a keyframe was encoded for other reasons. Other reasons include the kf_max_dist config and forced keyframes under high load. Would it be possible to detect keyframes and do the reset by looking at encoder output instead?

Otherwise, for instance with a 60fps constant-framerate stream and a 1600ms timeslice, you'd get keyframes at the timestamps:
[ 1000ms (max_dist)
, 1600ms (timeslice)
, 2000ms (max_dist)
, 3000ms (max_dist)
, 3200ms (timeslice)
, etc.
]
This would be unexpected.

::: dom/media/encoder/VP8TrackEncoder.h
@@ +81,5 @@
>  
>    /**
> +   * A duration of non-key frames in milliseconds.
> +  */
> +  StreamTime mNormalFramesDuration;

How about storing this in units of the track rate? That's also what the type `StreamTime` suggests. It has the benefit of avoiding per-frame drift due to rounding.

I'd also suggest the name mDurationSinceLastKeyframe.
Attachment #8943333 - Flags: review?(apehrson) → review-
Assignee: nobody → john357smith
Status: NEW → ASSIGNED
(Assignee)

Comment 63

a year ago
Thanks for a review. Regarding the keyframe interval - I'm aware of possibility to have key frame more often than desired but the worse thing could happen is that an output stream would be a slightly bigger (because of more keyframes). But for MediaRecorder you still get a correct timeslice intervals. And as Bryce wrote in comment #20 the principle of flushing a correct interval must be fixed on other place this is still only workaround.
Flags: needinfo?(apehrson)
I agree it is a workaround for some cases (short timeslice). But what we don't want is to observably affect *other* cases.

[1] should be a good place to reset the non-keyframe duration counter.


[1] https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/dom/media/encoder/VP8TrackEncoder.cpp#268
Flags: needinfo?(apehrson)
(Assignee)

Comment 65

a year ago
OK, understood I will prepare a modified patchset. Thanks.
(Assignee)

Comment 66

a year ago
Posted patch patchset_20180124.patch (obsolete) — Splinter Review
Attached you can find a modified patch according your comments. Just one notice regarding the reseting a non-keyframe interval. You suggested to reset a counter on the output from encoder but that means that if there is some extra key frame the non-keyframe interval will be delayed by number of frames which are in the encoder buffer already (usually 2). I'm not sure how long encoder buffer could be but for some cases it could affect the resulting blob duration.
Attachment #8943333 - Attachment is obsolete: true
Attachment #8945184 - Flags: review?(apehrson)
Comment on attachment 8945184 [details] [diff] [review]
patchset_20180124.patch

Review of attachment 8945184 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

I'll get this up on our test machines and a unit test or two written. Hang tight.


(In reply to john357smith from comment #66)
> Just one
> notice regarding the reseting a non-keyframe interval. You suggested to
> reset a counter on the output from encoder but that means that if there is
> some extra key frame the non-keyframe interval will be delayed by number of
> frames which are in the encoder buffer already (usually 2). I'm not sure how
> long encoder buffer could be but for some cases it could affect the
> resulting blob duration.

Right, that's probably fine. Let's call this a best effort workaround for now.

We could make it more accurate with some more bookkeeping, but for now, this is a small gap leading to some inaccuracy. Tracking the non-keyframe duration in milliseconds is another potential source of rounding errors.
Attachment #8945184 - Flags: review?(apehrson) → review+
Depends on: 1433062
Can you put up a patch that has your metadata (name, email, etc.) in it as well?

Something like `hg log -pr. > bug_1411857.patch` should do it.
Flags: needinfo?(john357smith)
(In reply to Andreas Pehrson [:pehrsons] from comment #67)
> (In reply to john357smith from comment #66)
> > Just one
> > notice regarding the reseting a non-keyframe interval. You suggested to
> > reset a counter on the output from encoder but that means that if there is
> > some extra key frame the non-keyframe interval will be delayed by number of
> > frames which are in the encoder buffer already (usually 2). I'm not sure how
> > long encoder buffer could be but for some cases it could affect the
> > resulting blob duration.
> 
> Right, that's probably fine. Let's call this a best effort workaround for
> now.

I'm gonna buckle here. Writing the unittest got really hard when a keyframe on the output resets the non-keyframe duration count.

Because we disable the encoder keyframe policy on a keyframe interval in Init() we can skip the reset if we have an invariant that disallows dynamic changes to the keyframe interval (i.e., updating it after Init()).

So, let's:

- Remove the non-keyframe duration reset when frames come out of the encoder (sorry for this), and
- Either:
  - `MOZ_ASSERT(!mInitialized);` in VideoTrackEncoder::SetKeyFrameInterval, or
  - Pass the keyframe interval in the constructor and make the member const


Could you update the patch with this as well?
Right, we still have to cover the case when the timeslice is large.

How about we scrap the auto keyframe policy altogether and always encode keyframes by enforcing them ourselves?

The keyframe interval then becomes `DEFAULT_KEYFRAME_INTERVAL_MS` by default, and in SetKeyFrameInterval it becomes `std::min(aKeyFrameInterval, DEFAULT_KEYFRAME_INTERVAL_MS)`.

Then we can scrap what I said about asserting or passing it to the constructor. This would allow dynamic changes. Neat!


We could still keep the kf policy at VPX_KF_AUTO, but with a larger kf_max_dist (let's say 600?), just to allow the encoder to encode a keyframe on say, scene changes.

We should probably make `DEFAULT_KEYFRAME_INTERVAL_MS` be `1000`. That's what we originally had (interval 30 with fixed fps 30). It was later changed to 60 when we moved to variable framerate and some devices would be 60fps.
Comment on attachment 8945184 [details] [diff] [review]
patchset_20180124.patch

Review of attachment 8945184 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/encoder/VP8TrackEncoder.cpp
@@ +662,5 @@
> +
> +      // Sum duration of non-key frames and force keyframe if exceeded the given keyframe interval
> +      if (mKeyFrameInterval > 0)
> +      {
> +        mDurationSinceLastKeyframe += chunk.GetDuration();

This is a bug. Because you're adding to the non-keyframe duration before the comparison to the interval, you're essentially encoding a key frame after a frame's *end time* is beyond the interval.

It needs to be the start time. So, first comparison, then add the chunk duration.
(Assignee)

Comment 73

a year ago
(In reply to Andreas Pehrson [:pehrsons] from comment #70)
> (In reply to Andreas Pehrson [:pehrsons] from comment #67)
> > (In reply to john357smith from comment #66)
> > > Just one
> > > notice regarding the reseting a non-keyframe interval. You suggested to
> > > reset a counter on the output from encoder but that means that if there is
> > > some extra key frame the non-keyframe interval will be delayed by number of
> > > frames which are in the encoder buffer already (usually 2). I'm not sure how
> > > long encoder buffer could be but for some cases it could affect the
> > > resulting blob duration.
> > 
> > Right, that's probably fine. Let's call this a best effort workaround for
> > now.
> 
> I'm gonna buckle here. Writing the unittest got really hard when a keyframe
> on the output resets the non-keyframe duration count.
> 
> Because we disable the encoder keyframe policy on a keyframe interval in
> Init() we can skip the reset if we have an invariant that disallows dynamic
> changes to the keyframe interval (i.e., updating it after Init()).
> 
> So, let's:
> 
> - Remove the non-keyframe duration reset when frames come out of the encoder
> (sorry for this), and
> - Either:
>   - `MOZ_ASSERT(!mInitialized);` in VideoTrackEncoder::SetKeyFrameInterval,
> or
>   - Pass the keyframe interval in the constructor and make the member const
> 
> 
> Could you update the patch with this as well?

Thanks. I will prepare a new patch.

What do you mean by disabling the encoder keyframe policy? If I'm correct in code a keyframe policy "AUTO" is still here for undefined or zero timeslice intervals:

  // for defined keyframe (>0) interval we force keyframes manually so we can disable automatic keyframe placing
  if (mKeyFrameInterval > 0) {
    config.kf_mode = VPX_KF_DISABLED;
  } else {
    config.kf_mode = VPX_KF_AUTO;
  }

A fixed keyframe interval is forced only for defined timeslice to keep a previous functionality of encoder not changed (mKeyFrameInterval is set from MediaRecorder}.
Flags: needinfo?(john357smith) → needinfo?(apehrson)
Duplicate of this bug: 1427181
(In reply to john357smith from comment #73) 
> Thanks. I will prepare a new patch.
> 
> What do you mean by disabling the encoder keyframe policy? If I'm correct in
> code a keyframe policy "AUTO" is still here for undefined or zero timeslice
> intervals:
> 
>   // for defined keyframe (>0) interval we force keyframes manually so we
> can disable automatic keyframe placing
>   if (mKeyFrameInterval > 0) {
>     config.kf_mode = VPX_KF_DISABLED;
>   } else {
>     config.kf_mode = VPX_KF_AUTO;
>   }
> 
> A fixed keyframe interval is forced only for defined timeslice to keep a
> previous functionality of encoder not changed (mKeyFrameInterval is set from
> MediaRecorder}.

Thanks. Could you also look at making the unittests in bug 1433062 comment 9 pass?

With disabling the keyframe policy I meant always doing `config.kf_mode = VPX_KF_DISABLED;`. But then in comment 71 I reasoned with myself about keeping it but increasing the max interval, so that we under normal operation encode all keyframes by forcing them, but still allowing it to encode a keyframe for other reasons, such as quality of the video. That said without knowing if it will actually look at the quality aspect, but from an API standpoint it seems reasonable.

Per comment 71 we'd then for undefined or zero timeslice intervals use a default keyframe interval in the encoder of, say, 1 second.
Flags: needinfo?(apehrson)
(Assignee)

Comment 76

a year ago
Posted patch patchset_20180128.patch (obsolete) — Splinter Review
New patchset is attached. I modified a comparison (>= mKeyFrameInterval) to be more precise and there is a question how to handle a non-defined keyframe interval (when SetKeyFrameInterval is not called). Currently there is a condition "mKeyFrameInterval > 0" which completely skip a manual keyframe placing so it's only on automatic mode with MAX_KEYFRAME_INTERVAL set to 600 (frames) - is that behaviour wanted?

Thanks.
Attachment #8945184 - Attachment is obsolete: true
Attachment #8946156 - Flags: review?(apehrson)
Comment on attachment 8946156 [details] [diff] [review]
patchset_20180128.patch

Review of attachment 8946156 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, really close now!

First of all, don't include my tests in this patch. Keep those patches in your local tree and put your patch on top. I'll land both bugs at the same time.

It'd also be good if you included the author info and title per comment 68 so I can use those when landing.

And a comment on fixing the issue with the last test inline.

::: dom/media/encoder/TrackEncoder.h
@@ +402,4 @@
>      , mEncodedTicks(0)
>      , mVideoBitrate(0)
>      , mFrameDroppingMode(aFrameDroppingMode)
> +    , mKeyFrameInterval(0)

Set this to DEFAULT_KEYFRAME_INTERVAL_MS and that will solve your last issue.

To get access to that `static const int` I'd just move this definition to the cpp file.

::: dom/media/encoder/VP8TrackEncoder.cpp
@@ +210,5 @@
>    config.rc_buf_optimal_sz = 600;
>    config.rc_buf_sz = 1000;
>  
> +  // we set key frame interval to automatic and later manually
> +  // force key frame by setting VPX_EFLAG_FORCE_KF when mKeyFrameInterval > 0 

Trailing space

@@ +654,5 @@
>        }
> +
> +      // Sum duration of non-key frames and force keyframe if exceeded the given keyframe interval
> +      if (mKeyFrameInterval > 0)
> +      { 

Trailing space
Attachment #8946156 - Flags: review?(apehrson) → review-
(Assignee)

Comment 78

a year ago
OK, test patch removed, metadata added, VideoTrackEncoder() definition moved to TrackEncoder.cpp (hope this is what you meant by moving defition to cpp file) and trailing spaces removed. Now all tests passed.

Thanks.
Attachment #8946156 - Attachment is obsolete: true
Attachment #8946385 - Flags: review?(apehrson)
Comment on attachment 8946385 [details] [diff] [review]
patchset_20180129.patch

Review of attachment 8946385 [details] [diff] [review]:
-----------------------------------------------------------------

Great work, thanks! Really appreciating your endurance here.

It passes locally for me. Here it is on try for other tests and platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=741bc9627e637a8088c08d785f6ca88ab116d4ac

I hope you don't mind me giving it a more descriptive commit summary.
Attachment #8946385 - Flags: review?(apehrson) → review+

Comment 80

a year ago
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69cd5e896f56
Set a custom time-based keyframe interval on VideoTrackEncoder. r=pehrsons
(Assignee)

Comment 81

a year ago
Thank you for committing this patch!

Just one last thing - a two months ago I raised a bug https://bugzilla.mozilla.org/show_bug.cgi?id=1424416 regarding the audio problem in webm file but till now nobody has looked at it. Could you help me to get somebody to look at it?

Thanks.
Flags: needinfo?(apehrson)

Comment 82

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/69cd5e896f56
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(In reply to john357smith from comment #81)
> Thank you for committing this patch!
> 
> Just one last thing - a two months ago I raised a bug
> https://bugzilla.mozilla.org/show_bug.cgi?id=1424416 regarding the audio
> problem in webm file but till now nobody has looked at it. Could you help me
> to get somebody to look at it?
> 
> Thanks.

I pinged one of our playback guys on that bug.
Flags: needinfo?(apehrson)
(Assignee)

Comment 84

a year ago
Thanks a lot.
status-firefox56: affected → wontfix
status-firefox57: affected → wontfix
status-firefox58: affected → wontfix
status-firefox59: --- → wontfix
Depends on: 1464268
You need to log in before you can comment on or make changes to this bug.