1207969 - Implement a PlatformDecoderModule by using OpenMax - Audio part

Status: RESOLVED DUPLICATE of bug 1224887

(Core :: Audio/Video: Playback, defect)

Description

[Jun-Da Zhou]

Sub part of bug 1203859. For intern.

Comment 1

[Jun-Da Zhou]

Attachment: Bug-1207969-v1.patch

Comment 2

[Alfredo Yang (:alfredo)]

Comment on attachment 8684087 [details] [diff] [review]
Bug-1207969-v1.patch

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

I don't full review the whole patch yet. Please try to follow coding style in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style in your codes.

::: dom/media/platforms/newdec/NewAudioDecoder.cpp
@@ +28,5 @@
> +  mTaskQueue = aTaskQueue;
> +  mCallback = aCallback;
> +  mType = aType;
> +  // new an instance for pPromise
> +  pPromise = new MozPromiseHolder<InitPromise>();

It is the wrong way to use promise. See https://dxr.mozilla.org/mozilla-central/source/dom/media/gtest/TestMozPromise.cpp.

@@ +35,5 @@
> +
> +RefPtr<MediaDataDecoder::InitPromise>
> +NewAudioDecoder::Init()
> +{
> +  currentAbstractThread = AbstractThread::GetCurrent();

You can get TaskQueue by AbstractThread::GetCurrent()->AsTaskQueue().

@@ +123,5 @@
> +  }
> +}
> +
> +void
> +NewAudioDecoder::ProcessDecode(RefPtr<MediaRawData> mSample)

Use aSample instead of mSample, mXXXX is for class member.

@@ +125,5 @@
> +
> +void
> +NewAudioDecoder::ProcessDecode(RefPtr<MediaRawData> mSample)
> +{
> +  BufferInfo *info = getFreeBufferInfo(OMX_DirInput);

If it runs out of buffer, the mSample will be discard. That may cause problem when playing audio.

@@ +139,5 @@
> +  }
> +}
> +
> +void
> +NewAudioDecoder::RetrieveBuffer(IOMX::buffer_id bufferID)

aBuffID

@@ +157,5 @@
> +  }
> +}
> +
> +void
> +NewAudioDecoder::PortSettingChanged(OMX_U32 portIndex)

aPortIndex, same as other places.

@@ +171,5 @@
> +
> +void
> +NewAudioDecoder::FreeAllBuffer(OMX_U32 portIndex)
> +{
> +  if(!isPortSettingChanged)

nit: always brace controlled statements, even a single-line consequent of an if else else.
if () {
}

@@ +199,5 @@
> +{
> +  if(isPortSettingChanged | isFlush | isShutdown){
> +    ssize_t index;
> +    BufferInfo *info = findBufferByID(OMX_DirOutput, bufferID, &index);
> +    if(info->mStatus!=BufferInfo::OWNED_BY_COMPONENT){

nit: add one space, "){" should be ") {".

@@ +201,5 @@
> +    ssize_t index;
> +    BufferInfo *info = findBufferByID(OMX_DirOutput, bufferID, &index);
> +    if(info->mStatus!=BufferInfo::OWNED_BY_COMPONENT){
> +      COREF_LOG("Weird!! the output buffer returned should be owned by component!!");
> +    }else{

Same as here.

if (...) {
} else if (...) {
} else {
}

::: dom/media/platforms/newdec/NewAudioDecoder.h
@@ +17,5 @@
> +#include <media/stagefright/foundation/ABuffer.h>
> +#include <media/stagefright/foundation/AString.h>
> +#include <android/log.h>
> +#include <utils/Vector.h>
> +#pragma GCC visibility push(default)

I believe you lack a line of "#pragma GCC visibility pop".

@@ +27,5 @@
> +#define COREF_LOG(...) __android_log_print(ANDROID_LOG_DEBUG, "NewAudioDecoder.cpp", __VA_ARGS__)
> +
> +namespace android
> +{
> +  template<class T>

Nit: don't indent code inside namespace.

@@ +29,5 @@
> +namespace android
> +{
> +  template<class T>
> +  static void InitOMXParams(T *params) {
> +      memset(params, 0, sizeof(T));

Nit: Two-space indentation. Same as other places.

@@ +45,5 @@
> +class MediaRawData;
> +
> +class PromiseHandle : public nsRunnable{
> +public:
> +  PromiseHandle( MozPromiseHolder<MediaDataDecoder::InitPromise> *mph, TrackInfo::TrackType *tt){

Several nits here.
It should be:

PromiseHandle(MozPromiseHolder<MediaDataDecoder::InitPromise>* aMph, TrackInfo::TrackType* aTt) {

@@ +46,5 @@
> +
> +class PromiseHandle : public nsRunnable{
> +public:
> +  PromiseHandle( MozPromiseHolder<MediaDataDecoder::InitPromise> *mph, TrackInfo::TrackType *tt){
> +	  pHolder = mph;

2 spaces indentation.

@@ +55,5 @@
> +	  pHolder->Resolve(*ptype,__func__);
> +	  return NS_OK;
> +  };
> +private:
> +  MozPromiseHolder<MediaDataDecoder::InitPromise> *pHolder;

mHolder

@@ +56,5 @@
> +	  return NS_OK;
> +  };
> +private:
> +  MozPromiseHolder<MediaDataDecoder::InitPromise> *pHolder;
> +  TrackInfo::TrackType *ptype;

mType

@@ +67,5 @@
> +
> +public:
> +
> +  NewAudioDecoder (const AudioInfo& aConfig,
> +		   FlushableTaskQueue* aTaskQueue,

Use space, don't use tab.

@@ +72,5 @@
> +                   MediaDataDecoderCallback* aCallback,
> +                   TrackInfo::TrackType aType);
> +
> +  virtual RefPtr<InitPromise>
> +  Init () override;

You don't need to declare virtual here because you already declare override. Same as other places. And you don't need to separate it to 2 lines because it doesn't exceed 80 characters.

@@ +101,5 @@
> +  void
> +  HandleSpecicCodecConfig();
> +
> +  void
> +  ProcessDecode(RefPtr<MediaRawData> mSample);

aSample, same as other places.

@@ +122,5 @@
> +      return NS_OK;
> +    };
> +  private:
> +    RefPtr<MediaRawData> mSample;
> +    NewAudioDecoder* mDecoder;

mDecoder is a raw pointer here, how do you ensure its life time is longer enough before it's deleted?

@@ +159,5 @@
> +						  duration.value(),
> +						  frames,
> +						  buffer.forget(),
> +						  mDecoder->mAudioChannels,
> +						  mDecoder->mAudioRate);

A lot of tabs in here!

@@ +188,5 @@
> +      FLUSH,
> +      OMXFLUSH,
> +      SETSTATE
> +    };
> +    MsgHandle(NewAudioDecoder *aDecoder, enum msgType aType, void *aData)

You don't need "enum msgType aType", just "msgType aType".

"void *aData" => "void* aData", same as other places.

@@ +196,5 @@
> +
> +    }
> +
> +    NS_IMETHOD Run() override{
> +      switch(mType){

nit:
switch (...) {
  case 1: {
    // When you need to declare a variable in a switch, put the block in braces
    int var;
    break;
  }
  case 2:
    ...
    break;
  default:
    break;
}

@@ +225,5 @@
> +      }
> +      return NS_OK;
> +    }
> +  private:
> +    NewAudioDecoder *mDecoder;

NewAudioDecoder* mDecoder;

@@ +227,5 @@
> +    }
> +  private:
> +    NewAudioDecoder *mDecoder;
> +    enum msgType mType;
> +    void *mData;

void* mData;

@@ +262,5 @@
> +  void CompleteShutdown();
> +
> +  void SetState(OMX_U32 state);
> +
> +  bool isPortSettingChanged = false;

It is not static variable, initialize it in constructor. Same as other place. And it should be named "mIsPortSettingChanged".

@@ +302,5 @@
> +  nsAutoCString mMimeType;
> +  RefPtr<FlushableTaskQueue> mTaskQueue;
> +  MediaDataDecoderCallback* mCallback;
> +  TrackInfo::TrackType mType;
> +  MozPromiseHolder<InitPromise>* pPromise;

We don't use promise in this way. Please see https://dxr.mozilla.org/mozilla-central/source/dom/media/gtest/TestMozPromise.cpp for promise usage.

@@ +307,5 @@
> +  sp<IOMX> mOMX;
> +  sp<NewObserver> mObserver;
> +  sp<android::MemoryDealer> mDealer[2];
> +  android::Vector<BufferInfo> mBuffers[2];
> +  AbstractThread * currentAbstractThread;

It is not good. You can get TaskQueue from codes like this:
https://dxr.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b/dom/media/platforms/agnostic/eme/EMEDecoderModule.cpp#261

@@ +309,5 @@
> +  sp<android::MemoryDealer> mDealer[2];
> +  android::Vector<BufferInfo> mBuffers[2];
> +  AbstractThread * currentAbstractThread;
> +  IOMX::node_id mNode;
> +  AString componentName;

nit: mComponentName

@@ +331,5 @@
> +//      new BlankMediaDataDecoder<BlankVideoDataCreator>(creator,
> +//                                                       aVideoTaskQueue,
> +//                                                       aCallback,
> +//                                                       TrackInfo::kVideoTrack);
> +//    return decoder.forget();

remove invalid codes.

@@ +343,5 @@
> +	const AudioInfo *ai = &aConfig;
> +	COREF_LOG("[AudioDecoder]OK, get Audio data=> mRate:%u\nmChannels:%u\nmBitDepth:%u\nmProfile:%d\nmExtendedProfile:%d\n",
> +				ai->mRate,ai->mChannels,ai->mBitDepth,ai->mProfile,ai->mExtendedProfile);
> +//    BlankAudioDataCreator* creator = new BlankAudioDataCreator(
> +//      aConfig.mChannels, aConfig.mRate, aConfig);

remove invalid codes.

@@ +353,5 @@
> +	                    TrackInfo::kAudioTrack);
> +//      new BlankMediaDataDecoder<BlankAudioDataCreator>(creator,
> +//                                                       aAudioTaskQueue,
> +//                                                       aCallback,
> +//                                                       TrackInfo::kAudioTrack);

remove invalid codes.

@@ +368,5 @@
> +
> +//  virtual bool
> +//  SupportsSharedDecoders(const VideoInfo& aConfig) const override {
> +//    return false;
> +//  }

remove invalid codes.

Comment 3

[Alfredo Yang (:alfredo)]

Attachment: omx_pdm

alfredo's draft version. :)

Comment 4

[Jun-Da Zhou]

Attachment: Bug-1207969-v2.patch -- some modification for conforming coding style

Comment 5

[Jun-Da Zhou]

Attachment: Bug-1207969-v3.patch