987568 - Refine the encoder codec selection in MediaEncoder::CreateEncoder

Status: RESOLVED INACTIVE

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

Description

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

In this function:
http://dxr.mozilla.org/mozilla-central/source/content/media/encoder/MediaEncoder.cpp#82
1. Have a common codec enable/disable preference.
2. Let mimeType ->codec mapping case more flexable.

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Benjamin, 
As talked, please have time to refine those code snap in this section.
Thanks.

Comment 2

[u459114]

Please come out a plan first and update your design doc or diagram here before jump into implementation.

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Benjaman,
As talked, 
Please have a draft and discuss with C.J/me, thanks.

Comment 4

[u459114]

M.M
Use a MIME type map
key - MIME string
value - {audio_codec creation func, video creation func, muxer creation func}

Have a unique map table for each platform.

Comment 5

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Please also construct gtest for this one, thanks.

Comment 6

[Benjamin Chen [:bechen]]

Attachment: WIP: bug-987568.wip.patch

1. Construct the mapping table for mimetype to encoder.
2. add Create() function for each xxxTrackEncoder and xxxWriter.

Can compile and record to a webm file on desktop build only.

Comment 7

[u459114]

Comment on attachment 8421608 [details] [diff] [review]
WIP: bug-987568.wip.patch

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

::: content/media/encoder/MediaEncoder.cpp
@@ +95,5 @@
> +
> +#define FACTORY_REF(name) Make##name
> +
> +#ifdef MOZ_WEBM_ENCODER
> +FACTORY_CREATE_VIDEO_ENCODER(VP8TrackEncoder)

VideoTrackEncoder*
VP8TrackEncoder::Create() // Or create another static function ::IsSupported() const
{
#ifdef MOZ_WEBM_ENCODER
  return new VP8TrackEncoder();
#else
  return nullptr;
#endif
}

And remove this macro

@@ +117,5 @@
> +  AudioTrackEncoder* (*audioCreateFunc)(void);
> +  ContainerWriter* (*containerCreateFunc)(int aTrackTypes, int option);
> +};
> +
> +static const EncoderInfo sEncoderInfo[] = {

Another prototype for your reference. Instead of using a list, I use a map with multiple coddec creation list
struct CreationInfo {
  VideoTrackEncoder* (*videoCreateFunc)(void);
  AudioTrackEncoder* (*audioCreateFunc)(void);
  ContainerWriter* (*containerCreateFunc)(int aTrackTypes, int option);
};

static const EncoderInfo sVIDEO_MP4_FallbackList[]  = {
  {&OmxVideoTrackEncoder::Create, &OmxAACAudioTrackEncoder::Create, &ISOMediaWriter::Create},
  {nullptr, nullptr, nullptr}
};

static const EncoderInfo  sAUDIO_3GPP_FallbackList[]  = {
  // video creation funtion can be null if this MIME type is audio only.
  {null_ptr, &OmxAACAudioTrackEncoder::Create, &ISOMediaWriter::Create},
  {nullptr, nullptr, nullptr}
};

class CodecMap {
public:
  typedef std::pair<nsString, EncoderInfo *> Pair;
  typedef std::map<nsString, EncoderInfo *>  Map;

  CodecMap()
  {
     mMap.insert(Pair(VIDEO_MP4, sVIDEO_MP4_FallbackList[0]));
     mMap.insert(Pair(AUDIO_3GPP, sAUDIO_3GPP_FallbackList[0]));
     // Insert all fllback list here.
  }
  
  // Return the most suitable creations in fallback list.
  EncoderInfo *Find(const nsString &aMIMEType)
  {
     CodecMap::iternator it = mMIMEMap.find(aMIMEType);
     
     if (it == mMIMIMap.end())
     {
       MOZ_ASSERT(falst); // Write erro log here
       return nullptr;
     }
     
     for (EncoderInfo *info = *it; info->containerCreateFunc; info++)
     {
       // VideoCodec creation test.
       // info->videoCreationFunc is not null means we need video codec for this mime type
       if (info->videoCreationFunc) 
       {
         TrackEncoder *codec = info->videoCreationFunc();
         // We need video codec, but this codec is not supported on this platform. Fallback.
         if (!codec)
           continue;
       } 

       // AudioCodec creation test.
       // info->audiooCreationFunc is not null means we need audio codec for this mime type
       if (info->audiooCreationFunc)
       {
         TrackEncoder *codec = info->audioCreationFunc();
         // We need audio codec, but this codec is not supported on this platform. Fallback.
         if (!codec)
           continue;
       }

       // Except past-the-end item in fallback list, muxer creation can not be null
       ContainerWriter *writer = info->containerCreateFun();
       if (!writer)
         continue;
   
       return info;
     }

     MOZ_ASSERT(falst); // Write erro log here
     return nullptr;
  }

private:
  Map mMIMEMap;
};

@@ +119,5 @@
> +};
> +
> +static const EncoderInfo sEncoderInfo[] = {
> +#ifdef MOZ_WEBM_ENCODER
> +  { VIDEO_WEBM, FACTORY_REF(VP8TrackEncoder), FACTORY_REF(VorbisTrackEncoder),

NS_LITERAL_STRING(VIDEO_WEBM)

@@ +154,5 @@
>      return nullptr;
>    }
> +  // hard code test webm
> +  const char *testmime = VIDEO_WEBM;
> +  for (int i = 0 ; i < sizeof(sEncoderInfo)/sizeof(EncoderInfo); ++i) {

#include "mozilla/ArrayUtils.h"
ArrayLength(sEncoderInfo)

@@ +155,5 @@
>    }
> +  // hard code test webm
> +  const char *testmime = VIDEO_WEBM;
> +  for (int i = 0 ; i < sizeof(sEncoderInfo)/sizeof(EncoderInfo); ++i) {
> +    if (strcmp(testmime, sEncoderInfo[i].mimetype) == 0) {

if (sEncoderInfo[i].mimetype == testmime )

@@ +164,5 @@
> +        audioEncoder = sEncoderInfo[i].audioCreateFunc();
> +      }
> +      if(sEncoderInfo[i].containerCreateFunc) {
> +        writer = sEncoderInfo[i].containerCreateFunc(aTrackTypes, 0);
> +      }

if (!videoEncoder || !audioEncoder || !writer)
{
  // Codec or writer is not supported on current platform. Fallback to another creation.
  continue;
}

@@ +171,1 @@
>    }

if (!videoEncoder || !audioEncoder || !writer)
{
  MOZ_ASSERT(....
  return fail to caller
}

::: content/media/webm/WebMWriter.h
@@ -40,5 @@
>   * This class accepts encoder to set audio or video meta data or
>   * encoded data to ebml Composer, and get muxing data through GetContainerData.
>   * The ctor/dtor run in the MediaRecorder thread, others run in MediaEncoder thread.
>   */
>  class WebMWriter : public ContainerWriter

MOZ_FILANL

@@ +62,5 @@
> +protected:
> +  // aTrackTypes indicate this muxer should multiplex into Video only or A/V foramt.
> +  // Run in MediaRecorder thread
> +  WebMWriter(uint32_t aTrackTypes);
> +

Make it private

Comment 8

[Benjamin Chen [:bechen]]

(In reply to C.J. Ku[:cjku] from comment #7)

> ::: content/media/webm/WebMWriter.h
> @@ -40,5 @@
> >   * This class accepts encoder to set audio or video meta data or
> >   * encoded data to ebml Composer, and get muxing data through GetContainerData.
> >   * The ctor/dtor run in the MediaRecorder thread, others run in MediaEncoder thread.
> >   */
> >  class WebMWriter : public ContainerWriter
> 
> MOZ_FILANL
> 
> @@ +62,5 @@
> > +protected:
> > +  // aTrackTypes indicate this muxer should multiplex into Video only or A/V foramt.
> > +  // Run in MediaRecorder thread
> > +  WebMWriter(uint32_t aTrackTypes);
> > +
> 
> Make it private

If we make the constructor to private, means that we can't derive the class anymore. It's not convenient for gtest's testcase. (class TestVP8TrackEncoder: public VP8TrackEncoder)

Comment 9

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

(In reply to Benjamin Chen [:bechen] from comment #8)
> If we make the constructor to private, means that we can't derive the class
> anymore. It's not convenient for gtest's testcase. (class
> TestVP8TrackEncoder: public VP8TrackEncoder)

Can't we favor composition over inheritance?

Comment 10

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #9)
> (In reply to Benjamin Chen [:bechen] from comment #8)
> > If we make the constructor to private, means that we can't derive the class
> > anymore. It's not convenient for gtest's testcase. (class
> > TestVP8TrackEncoder: public VP8TrackEncoder)
> 
> Can't we favor composition over inheritance?

Thanks, that's a good idea.

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8421608 [details] [diff] [review]
WIP: bug-987568.wip.patch

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

::: content/media/encoder/MediaEncoder.cpp
@@ +76,5 @@
>    }
>  
>  }
>  
> +

Remove this line

@@ +117,5 @@
> +  AudioTrackEncoder* (*audioCreateFunc)(void);
> +  ContainerWriter* (*containerCreateFunc)(int aTrackTypes, int option);
> +};
> +
> +static const EncoderInfo sEncoderInfo[] = {

Does mMap is mMIMEMap?
Does this handle the TrackType parameter from MediaRecorder?
If mimeType = "", do we want to insert the null mimeType map in mMap?

@@ +171,1 @@
>    }

In this case I prefer no assert here. 
User can disable codec in preference but it should be valid.

Comment 12

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

We can also use the Abstract Factory Pattern where MediaEncoder provides a factory for each specific codec/container implementation to register with. Then MediaEncoder would need to know nothing about the specific codec/container implementation and we can get rid of all headers such as VorbisTrackEncoder.h, VP8TrackEncoder.h, ISOMediaWriter.h and so on from MediaEncoder.cpp.

Comment 13

[Benjamin Chen [:bechen]]

Attachment: bug-987568.wip.patch

Based on CJ's suggestion at comment 7.

By the way, move constructor to private and modify gtest testcase not implement yet.

Comment 14

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

Comment on attachment 8422983 [details] [diff] [review]
bug-987568.wip.patch

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

::: content/media/encoder/MediaEncoder.cpp
@@ +82,5 @@
> +  AudioTrackEncoder* (*audioCreateFunc)(void);
> +  ContainerWriter* (*containerCreateFunc)(int aTrackTypes, int option);
> +};
> +
> +#ifdef MOZ_WEBM_ENCODER

|#ifdef MOZ_WEBM_ENCODER| and the like appear multiple times and spread across the code which makes the code complicated. Can the related code be put in the same ifdef closure?

@@ +130,5 @@
> +
> +class CodecMap {
> +public:
> +  typedef std::pair<nsString, const EncoderInfo*> Pair;
> +  typedef std::map<nsString, const EncoderInfo*> Map;

These typedefs don't have to be public.

@@ +201,5 @@
> +    if (info->audioCreateFunc &&
> +        (aTrackTypes & ContainerWriter::CREATE_AUDIO_TRACK)) {
> +      audioEncoder = info->audioCreateFunc();
> +    }
> +    // Special case for AUDIO_3GPP

Try to remove the special case here, otherwise you might need to modify the look-up code whenever you add a new table entry which just defeat the purpose of table look-up.

@@ +213,5 @@
> +      writer = info->containerCreateFunc(aTrackTypes, 0);
> +    }
> +    // Check the encoder and aTrackTypes.
> +    bool isVideoCreateSuccess = false;
> +    bool isAudioCreateSuccess = false;

Move is{Video|Audio}CreateSuccess above so you won't check aTrackTypes & ContainerWriter::CREATE_{VIDEO|AUDIO}_TRACK multiple times.

@@ +233,5 @@
> +
> +    // Creation fail, try the next iteration.
> +    writer = nullptr;
> +    audioEncoder = nullptr;
> +    videoEncoder = nullptr;

The logic seems to be wrong when MIME type is AUDIO_OGG and video track is specified. Ogg should ignore the video trackes but the code will return failure for video encoder can't be created.

Comment 15

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8422983 [details] [diff] [review]
bug-987568.wip.patch

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

::: content/media/encoder/MediaEncoder.cpp
@@ +215,5 @@
> +    // Check the encoder and aTrackTypes.
> +    bool isVideoCreateSuccess = false;
> +    bool isAudioCreateSuccess = false;
> +
> +    // Not set the CREATE_VIDEO_TRACK or create video encoder successfully.

Can compare the encoder/muxer pointer with EncoderInfo directly?
If success and return new MediaEncoder....

Comment 16

[Benjamin Chen [:bechen]]

(In reply to Randy Lin [:rlin] from comment #15)
> Comment on attachment 8422983 [details] [diff] [review]
> bug-987568.wip.patch
> 
> Review of attachment 8422983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/MediaEncoder.cpp
> @@ +215,5 @@
> > +    // Check the encoder and aTrackTypes.
> > +    bool isVideoCreateSuccess = false;
> > +    bool isAudioCreateSuccess = false;
> > +
> > +    // Not set the CREATE_VIDEO_TRACK or create video encoder successfully.
> 
> Can compare the encoder/muxer pointer with EncoderInfo directly?
> If success and return new MediaEncoder....
I'm not sure what the pointers you want to compare?
The EncoderInfo points to the create functions of video/audio/muxer. 
And then we call the create function to create them. Once the preference is closed, the create function returns null pointer.

Comment 17

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

If EncoderInfo has Audio/video/muxer create function. You will invoke all of them, then return the MediaEncoder object if all objects are created successfully as EncoderInfo defined.

Comment 18

[u459114]

Comment on attachment 8422983 [details] [diff] [review]
bug-987568.wip.patch

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

Please make MediaEncoder platform independent, hide all platform dependent code into CodecMap

::: content/media/encoder/MediaEncoder.cpp
@@ +79,3 @@
>  
> +struct EncoderInfo {
> +  VideoTrackEncoder* (*videoCreateFunc)(void);

How about rename to videoCreator?

@@ +162,5 @@
> +    }
> +
> +    return it->second;
> +  }
> +

Make const EncoderInfo *Find(const nsAString& aMIMEType) private:

Have a public function which return codec muxer directly
bool Find(const nsAString& aMIMEType, VideoTrackEncoder *&aVEncoder, AudioTrackEncoder *&aAEncoder,  ContainerWriter *&aWriter)
{
  // 1. Call Find(aMIMEType)
  // 2. Move codes in "MediaEncoder::CreateEncoder" to here.
}

Hide all details of codec creation inside CodecMap

@@ +200,5 @@
>      }
> +    if (info->audioCreateFunc &&
> +        (aTrackTypes & ContainerWriter::CREATE_AUDIO_TRACK)) {
> +      audioEncoder = info->audioCreateFunc();
> +    }

Logic here is complicate(Line #191~#204 + #220~#233)... it can be easy
1. User need audio encoding(aTrackTypes & ContainerWriter::CREATE_AUDIO_TRACK).
   info->audioCreateFunc need to be valid(none nullptr); otherwise, it has to be nullptr.
2. User need audio encoding(aTrackTypes & ContainerWriter::CREATE_VIDEO_TRACK).
   info->videoCreateFunc need to be valid(none nullptr); otherwise, it has to be nullptr
3. Container creator can never be nullptr.

// Video codec need.
if (aTrackTypes & ContainerWriter::CREATE_VIDEO_TRACK)
{
  if (!info->videoCreateFunc) {
    continue;
  }
  else {
    videoEncoder = info->videoCreateFunc();
  }
}
// Video codec does not need.
else 
{
  if (info->videoCreateFunc)
    continue;
}

And move these logic into CodeMap

@@ +205,5 @@
> +    // Special case for AUDIO_3GPP
> +#ifdef MOZ_OMX_ENCODER
> +    if (aMIMEType.EqualsLiteral(AUDIO_3GPP)) {
> +      writer = info->containerCreateFunc(aTrackTypes,
> +                                         ISOMediaWriter::TYPE_FRAG_3GP);

Can we pass ISOMediaWriter::TYPE_FRAG_3GP in handshake time instead of muxer creation time and remove adhoc code here?

And, move all #ifdef code into CodecMap, MediaEncoder is nice to be platform independent.

@@ +229,5 @@
> +    if (writer && isVideoCreateSuccess && isAudioCreateSuccess) {
> +      // Creation finished, break the for loop.
> +      break;
> +    }
> +

Line 220 ~ 233 is not needed

Comment 19

[u459114]

(In reply to JW Wang [:jwwang] from comment #12)
> We can also use the Abstract Factory Pattern where MediaEncoder provides a
> factory for each specific codec/container implementation to register with.
> Then MediaEncoder would need to know nothing about the specific
> codec/container implementation and we can get rid of all headers such as
> VorbisTrackEncoder.h, VP8TrackEncoder.h, ISOMediaWriter.h and so on from
> MediaEncoder.cpp.
From my understanding, why bechen doesn't apply this pattern is because of compile option, please have a discussion with him regarding to use the pattern.

Comment 20

[Benjamin Chen [:bechen]]

Attachment: bug-987568.v01.patch

1. MediaRecorder.cpp: if the mimetype is AUDIO_3GPP, set the trackType to audio.

2. MIMETypeEncoderMap::Find : Check the mimetype first, if the mimetype is not null, we check the trackType precisely. Otherwise, if the mimetype is null, we try to create the encoder as possible as we can even it's audio only or video only.

3. MIMETypeEncoderMap::CheckMuxerCapability : This function is to check the container writer to see if it has the ability to output audio only or video only data.

4. ContainerWriter::QueryMuxerCapability : a virtual function that returns muxer's capability.

5. ISOMediaWriterWrapper : a new file and new class to wrap the ISOMediaWriter because the source file is only available in "gonk".

Comment 21

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8426152 [details] [diff] [review]
bug-987568.v01.patch

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

::: content/media/encoder/ContainerWriter.h
@@ +32,5 @@
>    };
> +  enum {
> +    AUDIO_TRACK_ONLY = 1 << 0,
> +    VIDEO_TRACK_ONLY = 1 << 1,
> +    BOTH_AUDIO_VIDEO = 1 << 2,

Do we need the BOTH_AUDIO_VIDEO flag?

::: content/media/encoder/MediaEncoder.cpp
@@ +70,5 @@
> +  VideoTrackEncoder* (*videoCreator)(void);
> +  AudioTrackEncoder* (*audioCreator)(void);
> +  ContainerWriter* (*containerCreator)(int aTrackTypes);
> +};
> +

Comment on this.

@@ +100,5 @@
> +       &ISOMediaWriterWrapper::CreateFRAG3GP},
> +      {nullptr, nullptr, nullptr}
> +    };
> +    mMIMEMap.insert(Pair(NS_LITERAL_STRING(VIDEO_MP4),
> +                         sVIDEO_MP4_FallbackList));

Move this before // Priority list for AUDIO_3GPP.

@@ +125,5 @@
> +    };
> +    mMIMEMap.insert(Pair(NS_LITERAL_STRING(""), sDEFAULT_FallbackList));
> +  }
> +
> +  bool Find(const nsAString& aMIMEType, int aTrackTypes,

Could you have a better function name? 
You already have this one.
  const EncoderInfo *Find(const nsAString& aMIMEType)

@@ +233,5 @@
> +    }
> +    return true;
> +  }
> +
> +  Map mMIMEMap;

Put data members together.

::: content/media/ogg/OggWriter.h
@@ +22,5 @@
>  {
>  public:
> +  static ContainerWriter* Create(int aTrackTypes);
> +
> +  // TODO: fix me.

Does our writer have the ability to mux the video data?

Comment 22

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

Comment on attachment 8426152 [details] [diff] [review]
bug-987568.v01.patch

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

::: content/media/encoder/ISOMediaWriterWrapper.h
@@ +13,5 @@
> +namespace mozilla {
> +
> +class ContainerWriter;
> +// Dummy class
> +class ISOMediaWriterWrapper

Move the implementation to a CPP file so that it won't expose |ifdef|s to its user. Also you can extend the idea of this wrapper to remove the dummy classes below.

::: content/media/encoder/MediaEncoder.cpp
@@ +18,1 @@
>  #include "OmxTrackEncoder.h"

This include is valid only when MOZ_OMX_ENCODER is defined.

@@ +132,5 @@
> +            nsAutoPtr<ContainerWriter> &aWriter)
> +  {
> +    // Find the EncoderInfo list by aMIMEType.
> +    const EncoderInfo *info = Find(aMIMEType);
> +    NS_ENSURE_TRUE(info, nullptr);

replace |nullptr| with |false| for this function returns a bool instead of a pointer.

@@ +140,5 @@
> +      for (const EncoderInfo* i = info; i->containerCreator; ++i) {
> +        aWriter = nullptr;
> +        aVEncoder = nullptr;
> +        aAEncoder = nullptr;
> +        // aTrackTypes indicate that we need a video encoder.

The logic seems wrong. |aTrackTypes| indicates the track types (audio or video) contained in the stream. The user can choose to record audio only when the stream contains both audio and video tracks.

@@ +216,5 @@
> +    NS_ENSURE_TRUE(aWriter && (aVEncoder || aAEncoder), false);
> +
> +    int writerCapability = aWriter->QueryMuxerCapability();
> +    // Muxer doesn't support video only but we create a video encoder only.
> +    if (!(writerCapability | ContainerWriter::VIDEO_TRACK_ONLY) &&

Are these enum values intended to be bit flags or numeric values? If they are numeric values, they should be compared using "==" or "!=".

@@ +262,5 @@
> +                              videoEncoder, audioEncoder, writer);
> +  NS_ENSURE_TRUE(res, nullptr);
> +
> +  // Check the encoders and muxer again.
> +  NS_ENSURE_TRUE((audioEncoder || videoEncoder) && writer, nullptr);

This is already checked in CheckMuxerCapability() above.

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +162,5 @@
> +VideoTrackEncoder*
> +OmxVideoTrackEncoder::Create()
> +{
> +#ifdef MOZ_OMX_ENCODER
> +  if (MediaEncoder::IsOMXEncoderEnabled()) {

Remove IsOMXEncoderEnabled() from MediaEncoder and move the implementation here so that you don't need to include "MediaEncoder.h". Also remove |#ifdef MOZ_OMX_ENCODER| from MediaEncoder which should know as less as a particular encoder implementation.

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +569,5 @@
> +/* static */
> +VideoTrackEncoder*
> +VP8TrackEncoder::Create()
> +{
> +  return new VP8TrackEncoder();

Need to test |MediaEncoder::IsWebMEncoderEnabled()|?

::: content/media/encoder/fmp4_muxer/ISOMediaWriter.h
@@ +22,5 @@
> +  static ContainerWriter* CreateFRAG3GP(int aTrackTypes);
> +
> +  // TODO: fix me.
> +  int QueryMuxerCapability() MOZ_FINAL MOZ_OVERRIDE {
> +    return AUDIO_TRACK_ONLY | VIDEO_TRACK_ONLY | BOTH_AUDIO_VIDEO;

Shouldn't BOTH_AUDIO_VIDEO == AUDIO_TRACK_ONLY | VIDEO_TRACK_ONLY hold?

::: content/media/webm/moz.build
@@ +6,5 @@
>  
>  EXPORTS += [
>      'WebMDecoder.h',
>      'WebMReader.h',
> +    'WebMWriter.h'

See the comment above. You should be able to extend the idea of ISOMediaWriterWrapper to remove the dummy classes without moving WebMWriter.h to public exports.

Comment 24

[u459114]

Comment on attachment 8426152 [details] [diff] [review]
bug-987568.v01.patch

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

Please check my comment

::: content/media/encoder/MediaEncoder.cpp
@@ +72,5 @@
> +  ContainerWriter* (*containerCreator)(int aTrackTypes);
> +};
> +
> +class MIMETypeEncoderMap {
> +public:

There are many kinds of MIME type; we only care about two of them here: type video and type audio

Pre-condition:
No CREATE_VIDEO_TRACK + No CREATE_AUDIO_TRACK = not ok. It's not quite right. For recording, we need at last one source track.

video MIME type + CREATE_VIDEO_TRACK + CREATE_AUDIO_TRACK = ok
video MIME type + CREATE_AUDIO_TRACK = Not ok. Recode a video clip with no video make no sense.
video MIME type + CREATE_VIDEO_TRACK = ok. But, need to check capability of muxer.

audio MIME type + CREATE_VIDEO_TRACK + CREATE_AUDIO_TRACK = ok. Skip source video.
audio MIME type + CREATE_AUDIO_TRACK = ok
audio MIME type + CREATE_VIDEO_TRACK = not ok. No source audio.

no MIME type + CREATE_VIDEO_TRACK + CREATE_AUDIO_TRACK = ok
no MIME type + CREATE_VIDEO_TRACK = ok
no MIME type + CREATE_AUDIO_TRACK = ok

Define these rules in private member functions.
bool ValidateVideoEncoder(aVEncoder, aMIMEType)
{
  if (is_video_mimetype)
  else if (is_audio_mimetype)
  else // default mimetype
 
}

@@ +113,5 @@
> +    mMIMEMap.insert(Pair(NS_LITERAL_STRING(AUDIO_OGG),
> +                         sAUDIO_OGG_FallbackList));
> +
> +    // Insert null string list. Note that the order in the list is meaningful
> +    // because we will check the list in a loose condition.

// Platform default encoder and muxer.
// If a user does not choice a MIME type, it tells media encoder to choice
// default encoder/muxer according to current platform.

@@ +186,5 @@
> +          return true;
> +        }
> +        // Creation fail, try the next iteration.
> +      }
> +    }

Too many dup code in if and else statement.

for (const EncoderInfo* i = info; i->containerCreator; ++i) {
  nsAutoPtr<VideoTrackEncoder> vEncoder;
  nsAutoPtr<AudioTrackEncoder> aEncoder
  nsAutoPtr<ContainerWriter>   writer;

  aVEncoder = nullptr;
  aAEncoder = nullptr;
  
  // Create video encoder
  if (aTrackTypes & ContainerWriter::CREATE_VIDEO_TRACK) {
    vEncoder = i->videoCreator();
    if (!ValidateVideoEncoder(vEncoder, aMIMEType))
      continue;
  }

  // Create audio encoder
  if (aTrackTypes & ContainerWriter::CREATE_AUDIO_TRACK) {
    aEncoder = i->audioCreator();
    if (!ValidateAudeoEncoder(aEncoder, aMIMEType))
      continue;
  }

  // Create container.
  aWriter = i->containerCreator(aTrackTypes);
 
  // Check whether aWriter accept aVEncoder && aAEncoder.
  if (CheckMuxerCapability(aVEncoder, aAEncoder, aWriter)) {
    LOG(PR_LOG_DEBUG, ("Null mimetype create successfully."));
    aVEncoder = vEncoder.forget();
    aAEncoder = aEncoder.forget();
    aWriter = writer.forget();
    return true;
  }
}

Comment 25

[Benjamin Chen [:bechen]]

Attachment: bug-987568.v02.patch

Follow the comment 22, comment 24.
Also add mimetype field in EncoderInfo in order to return the final mimetype
if the input mimetypr is null string.

Comment 26

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

Comment on attachment 8438131 [details] [diff] [review]
bug-987568.v02.patch

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

Whenever a new encoder/muxer is added, you have to modify code in multiple places (eg: EncoderWrapper and the table entries in MediaEncoder.cpp). Can you explain how this patch improves the code maintenance/readablity/flexibility?

::: content/media/encoder/MediaEncoder.cpp
@@ +187,5 @@
> +    return it->second;
> +  }
> +
> +  // Check the muxer and a/v encoders are compatible or not.
> +  bool CheckMuxerCapability(nsAutoPtr<VideoTrackEncoder> &aVEncoder,

|aVEncoder| is used to check nullity only. You can pass a boolean instead of a smart pointer.

@@ +213,5 @@
> +    }
> +    return true;
> +  }
> +
> +  bool ValidateVideoEncoder(const nsAString& aMIMEType,

ValidateVideoEncoder and ValidateAudioEncoder are similar. They can be combined into one function. |aVEncoder| is used to check nullity only. You can pass a boolean instead of a smart pointer.

::: content/media/encoder/moz.build
@@ +26,5 @@
>      EXPORTS += ['OmxTrackEncoder.h']
>      UNIFIED_SOURCES += ['OmxTrackEncoder.cpp']
>  
>  if CONFIG['MOZ_OPUS']:
> +    EXPORTS += ['OpusTrackEncoder.h',]

wrong comma.

::: content/media/webm/WebMWriter.h
@@ +45,5 @@
>  {
>  public:
> +  static ContainerWriter* Create(int aTrackTypes);
> +
> +  static bool IsWebMEncoderEnabled();

This function doesn't need to be exposed in the header. A header should only expose what is needed by the user.

Comment 27

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #26)
> Comment on attachment 8438131 [details] [diff] [review]
> bug-987568.v02.patch
> 
> Review of attachment 8438131 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Whenever a new encoder/muxer is added, you have to modify code in multiple
> places (eg: EncoderWrapper and the table entries in MediaEncoder.cpp). Can
> you explain how this patch improves the code
> maintenance/readablity/flexibility?
> 
hmm, good question...

How about moving the whole staff to EncoderWrapper.h EncoderWrapper.cpp, at least we could rarely touch MediaEncoder.cpp, only modify the EncoderWrapper if add a new encoder/muxer or some ifdef/preference/.

Comment 28

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

How is that different from the old way where we keep the whole stuff in MediaEncoder.cpp?

Comment 29

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8438131 [details] [diff] [review]
bug-987568.v02.patch

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

::: content/media/encoder/ContainerWriter.h
@@ +33,5 @@
> +  enum {
> +    AUDIO_TRACK_ONLY = CREATE_AUDIO_TRACK,
> +    VIDEO_TRACK_ONLY = CREATE_VIDEO_TRACK,
> +    BOTH_AUDIO_VIDEO = 1 << 2,
> +  };

Could we combine CREATE_AUDIO_TRACK & AUDIO_TRACK_ONLY ?

::: content/media/encoder/EncoderWrapper.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/

I think this objects isn't really a kind of Wrapper...Just an object creater.

::: content/media/webm/WebMWriter.cpp
@@ +80,5 @@
> +
> +/* static */
> +bool
> +WebMWriter::IsWebMEncoderEnabled()
> +{

Ensure it's running in mainthread.

Comment 30

[Alfredo Yang (:alfredo)]

Comment on attachment 8438131 [details] [diff] [review]
bug-987568.v02.patch

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

I think the components should try to keep themselves clear as much as possible, not to touch the reference setting or environment settings.
It's probably more clear and easier to debug from my view.

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +170,5 @@
> +  if (IsOMXEncoderEnabled()) {
> +    return new OmxVideoTrackEncoder();
> +  }
> +  return nullptr;
> +}

It should be caller's job to check the perference before creating OMX encoder, not encoder itself.

::: content/media/encoder/fmp4_muxer/ISOMediaWriter.cpp
@@ +232,5 @@
> +    return new ISOMediaWriter(aTrackTypes, TYPE_FRAG_MP4);
> +  }
> +#endif
> +  return nullptr;
> +}

This component doesn't need to know if its encoder is OMX or not. It doesn't matter to ISOMediaWriter.

::: content/media/encoder/fmp4_muxer/ISOMediaWriter.h
@@ +23,5 @@
> +
> +  // TODO: fix me.
> +  int QueryMuxerCapability() MOZ_FINAL MOZ_OVERRIDE {
> +    return AUDIO_TRACK_ONLY | VIDEO_TRACK_ONLY | BOTH_AUDIO_VIDEO;
> +  };

It should not capability, it is restricted by input types, not its own capability.

Comment 31

[u459114]

Comment on attachment 8438131 [details] [diff] [review]
bug-987568.v02.patch

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

::: content/media/encoder/EncoderWrapper.cpp
@@ +18,5 @@
> +#include "OpusTrackEncoder.h"
> +#endif
> +
> +namespace mozilla {
> +

If you do want to create these wrappers, then there is no reason to have static Create in TrackEncoder and ContainerWriter

::: content/media/encoder/EncoderWrapper.h
@@ +12,5 @@
> +class VideoTrackEncoder;
> +class AudioTrackEncoder;
> +
> +// Wrapper classes
> +class ISOMediaWriterWrapper

class MP4WriterCreator && class F3GPPWriterCreater

@@ +19,5 @@
> +  static ContainerWriter* Create(int aTrackTypes);
> +  static ContainerWriter* CreateFRAG3GP(int aTrackTypes);
> +};
> +
> +class WebMWriterWrapper

Rename Wrapper to Creator

::: content/media/encoder/MediaEncoder.cpp
@@ +95,5 @@
> +       &OmxAACAudioTrackEncoderWrapper::Create,
> +       &ISOMediaWriterWrapper::CreateFRAG3GP},
> +      {NS_LITERAL_STRING(""), nullptr, nullptr, nullptr}
> +    };
> +    mMIMEMap.insert(Pair(NS_LITERAL_STRING(VIDEO_MP4),

Move #99 upto #90

Comment 32

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 33

[Peter Magyari (Desktop QA)]

Hi,

Will there be any updates on this issue JW Wang? It has been open for a really long time now.
It would be nice to close if it is no longer relevant.

Thanks!

Comment 34

[Paul Adenot (:padenot)]

Let's close as inactive. We're going to have a fresh look at encoder code when we implement Web Codecs this year.