[DropWebp] add enableDropFrame to webp play

[FieldWangHD]

The redesign of #2110 . @oprisnik
I modified the following code:

1. ImageDecodeOption add enableDropFrame flag. After this flag is turned on, there is no guarantee that WebP will play on time, but it will certainly not stuck the UI thread.
2. Storage aspects. When enableDropFrame is set, the first frame and other frames exist in different caches. The first frame is along with Fresco's BitmapCache, while the other frames are together. This is done to ensure that the first frame comes out in time.
3. Control aspects. When enableDropFrame is set, only the current frame to be drawn is prepared in the background. If the current frame is not drawn correctly, the frame will be drawn next time.
4. WebP result will be cached.
5. Add a fragment to display the WebP list.

[facebook-github-bot]

@woyunowuyuda has updated the pull request. View: changes

[facebook-github-bot]

@woyunowuyuda has updated the pull request. View: changes

[lambdapioneer]

Thank you very much for the pull-request @woyunowuyuda! I'll import it for our internal tests and get @oprisnik involved to have a look at the animation code :)

[facebook-github-bot]

@lambdapioneer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[FieldWangHD]

@lambdapioneer Thanks for review.If you are going to start internal tests,I suggest to use different webPs in RecyclerView.The fragment in my RP only used one webP(I have not that much webP res).

[oprisnik]

Thanks for looking into this. I think there is still room for simplification. This is a huge change and it should be doable with less invasive changes.

[oprisnik]

We shouldn't add multiple caches. What's the value of this?

[FieldWangHD]

When we scroll in a webP list, if we put all webP frames in one FrameCache, the outwindow webP is easier to be recycled. I used a otherFrameCahce to cache other frames and mAnimatedFrameCache to cache first frame. Then when we scroll in a webP list, the other frames can be recycled, the first frame remains, it can take less time for users to preview the image.

[oprisnik]

If you always want to keep the first frame (which should be an option in ImageDecodeOptions), I suggest to just hold a reference to the bitmap in the backend and close it when the backend is disposed. This would make the code changes way less invasive. It should also be a separate pull request since it's quite unrelated to the rest (and can be used standalone). I'm also not convinced I see the advantage of having the first frame all the time.

[oprisnik]

unrelated

[oprisnik]

This is a sensible approach. I don't think you need the rest of your changes. Only having this should already prevent not drawing on the UI thread. Please rename this to mAllowDecodingOnUiThread or something similar.

[oprisnik]

Why is this needed?

[oprisnik]

Why do you need this?

[oprisnik]

This is misleading. I think what you want to do is to keep the existing preparation strategy (i.e. prepare the next N frames ahead of time) but instead of scheduling the next frame in the drawable, re-schedule the same one. I had a prototype of AnimatedDrawable2 in the beginning that had 2 modes, one to delay, one to drop. This way, you don't need to change many Fresco-specific files and can just pass the flag down to the AnimatedDrawable instead. This should simplify the code a lot.

It looked like this:

  @Override
  public void draw(Canvas canvas) {
    if (mAnimationBackend == null) {
      return;
    }
    mAnimationListener.onAnimationFrame(this, mCurrentFrame);
    if (mCurrentFrame == 0 && mCurrentLoopCount != 0) {
      mAnimationListener.onAnimationRepeat(this);
    }
    // TODO: only draw if close to target frame render time, otherwise draw previous frame
    // in case something else invalidates this the animation would be too fast otherwise.
    long actualRenderTimeStart = now();
    // TODO: check if frame drawn
    boolean frameDrawn = mAnimationBackend.drawFrame(this, canvas, mCurrentFrame);
    long actualRenderTimeEnd = now();
    if (FLog.isLoggable(FLog.VERBOSE)) {
      FLog.v(TAG,
          "Animation jitter: %s ms. Render time: %s ms. Frame drawn: %s",
          actualRenderTimeStart - mTargetFrameRenderTime,
          actualRenderTimeEnd - actualRenderTimeStart,
          frameDrawn);
    }
    if (!mIsRunning) {
      return;
    }
    if (frameDrawn) {
      mTargetFrameRenderTime += mAnimationBackend.getFrameDurationMs(mCurrentFrame);
    } else if (mFrameDelayMethod == FRAME_DELAY_METHOD_DROP) {
        mTargetFrameRenderTime += mAnimationBackend.getFrameDurationMs(mCurrentFrame);
        // TODO: what if frame duration < 16ms? should we just skip it instead since 60fps should be enough?
        // Maybe add limitFPS setting
        int droppedFramesThreshold = 10;
        while (mTargetFrameRenderTime < actualRenderTimeEnd && droppedFramesThreshold > 0) {
          // we need to drop frames
          if (FLog.isLoggable(FLog.VERBOSE)) {
            FLog.v(TAG, "Dropped a frame. Count: %s", mDroppedFrames);
          }
          if (!increaseFrameCount()) {
            // TODO: onAnimationDone?
            mAnimationListener.onAnimationStop(this);
            return; // animation done
          }
          mDroppedFrames++;
          droppedFramesThreshold--;
          mTargetFrameRenderTime += mAnimationBackend.getFrameDurationMs(mCurrentFrame);
        }
      } else if (mFrameDelayMethod == FRAME_DELAY_METHOD_DELAY) {
        mTargetFrameRenderTime = now() + mAnimationBackend.getFrameDurationMs(mCurrentFrame);
      }
    scheduleNextFrameOrNotifyAnimationFinished();
  }

[oprisnik]

I don't think this should be needed.

[erikandre]

Closing since the comments by @oprisnik has not been addressed. Please feel free to comment here and also push and updated version and we can take it from there. Thanks!