1243611 - VP8TrackEncoder is not calling vpx_codec_encode correctly on EOS

Status: RESOLVED FIXED

(Core :: Audio/Video: Recording, defect, P2)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

While working on bug 1237848, I stumbled upon a use a vpx_codec_encode that may be wrong.

According to the documentation of vpx_codec_encode:
http://mxr.mozilla.org/mozilla-central/source/media/libvpx/vpx/vpx_encoder.h#920
> When the last frame has been passed to the encoder, this function should
> continue to be called, with the img parameter set to NULL. This will
> signal the end-of-stream condition to the encoder and allow it to encode
> any held buffers. Encoding is complete when vpx_codec_encode() is called
> and vpx_codec_get_cx_data() returns no data.

But in VP8TrackEncoder::GetEncodedTrack:
http://mxr.mozilla.org/mozilla-central/source/dom/media/encoder/VP8TrackEncoder.cpp#628
>   // End of stream, pull the rest frames in encoder.
>   if (EOS) {
>     VP8LOG("mEndOfStream is true\n");
>     mEncodingComplete = true;
>     if (vpx_codec_encode(mVPXContext, nullptr, mEncodedTimestamp,
>                          mEncodedFrameDuration, 0, VPX_DL_REALTIME)) {
>       return NS_ERROR_FAILURE;
>     }
>     GetEncodedPartitions(aData);
>   }

So vpx_codec_encode is only called once, and then vpx_codec_get_cx_data is called in a loop inside GetEncodedPartitions.
However according to the documentation (as I interpret it), vpx_codec_encode should be called repeatedly (with a NULL img) until vpx_codec_get_cx_data returns no more data.

Comment 1

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fd963e9de1f

Comment 2

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 1243611 - When EOS, call vpx_codec_encode correctly. r=rillian

Review commit: https://reviewboard.mozilla.org/r/36011/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36011/

Comment 3

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/36009/#review32635

::: dom/media/encoder/VP8TrackEncoder.h:59
(Diff revision 1)
>    nsresult GetEncodedPartitions(EncodedFrameContainer& aData);

Add return value if the vpx_codec_get_cx_data return null.

::: dom/media/encoder/VP8TrackEncoder.cpp
(Diff revision 1)
> -      (pkt->data.frame.pts == mEncodedTimestamp)) {

The mEncodedTimestamp is the timestamp of latest frame we sent into encoder (latest input timestamp).  The old code check it strictly would cause the frame be encoded "1 by 1" (1 input then 1 output ...).

For futher refactory, change the "1 by 1" to other, I should remove the check . And the code change should be harmless.

::: dom/media/encoder/VP8TrackEncoder.cpp:365
(Diff revision 1)
> -    VP8LOG("Converted an %s frame to I420\n");
> +    VP8LOG("Converted an %s frame to I420\n", yuvFormat.c_str());

Missing |yuvFormat.c_str()|.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8722400 [details]
MozReview Request: Bug 1243611 - When EOS, call vpx_codec_encode correctly. r=rillian

Not familiar with the code. Can you find rillian or roc?

Comment 5

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/36011/#review32825

::: dom/media/encoder/VP8TrackEncoder.cpp:631
(Diff revision 1)
> +    // Bug 1243611, keep calling vpx_codec_encode and vpx_codec_get_cx_data

Address comment 0.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8722400 [details]
MozReview Request: Bug 1243611 - When EOS, call vpx_codec_encode correctly. r=rillian

https://reviewboard.mozilla.org/r/36011/#review33003

r=me with nits addressed.

::: dom/media/encoder/VP8TrackEncoder.h:58
(Diff revision 1)
> +  //               vpx_codec_get_cx_data returns null for EOS detection.

NS_ERROR_NULL_POINTER is usually used for argument validation. I would have used a bool return, I think, so you could just

```
do {
   ...
} while(GetEncodedPartition(aData));
```

but I suppose this works too, and most of the other methods return nsresult.

::: dom/media/encoder/VP8TrackEncoder.cpp:206
(Diff revision 1)
> -        (uint64_t)FramesToUsecs(mEncodedTimestamp, mTrackRate).value());
> +        (uint64_t)FramesToUsecs(pkt->data.frame.pts, mTrackRate).value());

Why not call
```
videoData->SetTimeStamp((uint64_t)timestamp.value());
```
instead of invoking the `FramesToUSecs` ctor twice?

::: dom/media/encoder/VP8TrackEncoder.cpp:634
(Diff revision 1)
> +    while (rv != NS_ERROR_NULL_POINTER) {

This will loop forever if `GetEncodedPartitions()` is later changed to return other failure codes. If you're using nsresult, you need to handle that case as well to guard against the introduction of future bugs. Something like
```
while (rv != NS_ERROR_NULL_POINTER) {
  if (NS_FAILED(rv)) {
    return rv;
  }
  if (vpx_codec_encode(...
```

Comment 7

[Benjamin Chen [:bechen]]

Comment on attachment 8722400 [details]
MozReview Request: Bug 1243611 - When EOS, call vpx_codec_encode correctly. r=rillian

https://reviewboard.mozilla.org/r/36011/#review33353

Comment 8

[Benjamin Chen [:bechen]]

Comment on attachment 8722400 [details]
MozReview Request: Bug 1243611 - When EOS, call vpx_codec_encode correctly. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36011/diff/1-2/

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/185b5f9f98f5

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/185b5f9f98f5