Closed Bug 1243611 Opened 4 years ago Closed 4 years ago

VP8TrackEncoder is not calling vpx_codec_encode correctly on EOS

Categories

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

43 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: gerald, Assigned: bechen)

Details

Attachments

(1 file)

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.
Component: Audio/Video → Audio/Video: Recording
Rank: 25
Priority: -- → P2
Assignee: nobody → 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()|.
Attachment #8722400 - Flags: review?(jwwang)
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?
Attachment #8722400 - Flags: review?(jwwang)
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.
Attachment #8722400 - Flags: review?(giles)
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(...
```
Attachment #8722400 - Flags: review?(giles) → review+
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
Attachment #8722400 - Flags: review+
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/
Attachment #8722400 - Attachment description: MozReview Request: Bug 1243611 - When EOS, call vpx_codec_encode correctly. r? → MozReview Request: Bug 1243611 - When EOS, call vpx_codec_encode correctly. r=rillian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/185b5f9f98f5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.