1372080 - [EME] Reorder frames coming out of Widevine CDM

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, enhancement, P3)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

The next version of the Widevine CDM (970) has a new H.264 decoder, and it doesn't reorder frames it decodes. So we need to reorder manually.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1372080 - Reorder frames decoded by Widevine CDM.

The next version of the Widevine CDM (970) has a new H.264 decoder and it does
not appear to be outputing frames it decodes in presentation order, so we need
to reorder the frames output by the CDM.

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

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8876575 [details]
Bug 1372080 - Reorder frames decoded by Widevine CDM.

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

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8876575 [details]
Bug 1372080 - Reorder frames decoded by Widevine CDM.

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

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8876575 [details]
Bug 1372080 - Reorder frames decoded by Widevine CDM.

https://reviewboard.mozilla.org/r/147910/#review152276

::: dom/media/gmp/ChromiumCDMParent.h:19
(Diff revision 1)
>  #include "mozilla/RefPtr.h"
>  #include "nsDataHashtable.h"
>  #include "PlatformDecoderModule.h"
>  #include "ImageContainer.h"
>  #include "mozilla/Span.h"
> +#include "ReorderQueue.h"

those includes should be in alhabetical order...

::: dom/media/gmp/ChromiumCDMParent.h:132
(Diff revision 1)
>    ipc::IPCResult RecvResetVideoDecoderComplete() override;
>    ipc::IPCResult RecvDrainComplete() override;
>    void ActorDestroy(ActorDestroyReason aWhy) override;
>    bool SendBufferToCDM(uint32_t aSizeInBytes);
>  
> +  void ReorderAndReturnOutput(RefPtr<VideoData> aFrame);

You call ReorderAndReturnOutput(Move(v)) so make this take a rvalue RefPtr<VideoData>&&

Otherwise, the move is wasted.

::: dom/media/gmp/ChromiumCDMParent.cpp:745
(Diff revision 1)
>  
>  ipc::IPCResult
>  ChromiumCDMParent::RecvDecodedShmem(const CDMVideoFrame& aFrame,
>                                      ipc::Shmem&& aShmem)
>  {
> -  GMP_LOG("ChromiumCDMParent::RecvDecodedShmem(this=%p)", this);
> +  GMP_LOG("ChromiumCDMParent::RecvDecodedShmem(this=%p) t=%lld d=%lld",

that's going to do warning as error on some build, need to use PRId64 etc...

::: dom/media/gmp/ChromiumCDMParent.cpp:789
(Diff revision 1)
>  
>    return IPC_OK();
>  }
>  
> +void
> +ChromiumCDMParent::ReorderAndReturnOutput(RefPtr<VideoData> aFrame)

RefPtr<VideoData>&& aFrame

::: dom/media/gmp/ChromiumCDMParent.cpp:791
(Diff revision 1)
>  }
>  
> +void
> +ChromiumCDMParent::ReorderAndReturnOutput(RefPtr<VideoData> aFrame)
> +{
> +  if (mMaxRefFrames > 0) {

Personally, I would drop one level with something like:

if (mMaxRefFrames == 0) {
  ResolveIfExists(...)
  return;
}

....

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8876575 [details]
Bug 1372080 - Reorder frames decoded by Widevine CDM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147910/diff/3-4/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0690e322191e
Reorder frames decoded by Widevine CDM. r=jya

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0690e322191e

Comment 8

[Clancy_xie]

This bug may lead to the normal I-frame can not decode play it？

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

This bug caused the frame output by the Widevine CDM to be played in the wrong order. The frames were decoded correctly, but were not reordered into the correct display order.