Closed Bug 919905 Opened 11 years ago Closed 11 years ago

Media Recording - Refactor encoder architecture to support the video encoder module

Categories

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

Product:
Core
Component:
Audio/Video: Recording
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27

People

(Reporter: rlin, Assigned: rlin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 11 obsolete files)

check-in patch
22.50 KB, patch
Details | Diff | Splinter Review
Right now we want to add the video encoding function. But consider for the video encoder/muxer, we need to change the interface to for-fit the data transfer between the encoder and muxer. This bug would change some design of the original one and let video encoder can add in the encoder module.
randy, would you list which interface that you think out of date?
I would likely to change those things first. 1. GetHeader -> GetMetaData( EncodeMetaData aMeta) EncodeMetaData can be key-value or a structure, which used to store the meta data from codec like video:width, height, frameRate audio:SampleFreq, bitDepth, Channels, codec_private_data 2. GetEncodeTrack(nsTArray<unit8_t> aBuf, duration) -> GetEncodeTrack(EncodePacket aPacket) EncodePacket for storing the audio/video encoded packet. video should has I/P frame information, timestamp, audio should has duration. 3. WriteEncodeTrack(nsTArray<unit8_t> aBuf, duration) -> WriteEncodeTrack(EncodePacket aPacket) Same as point 2.
Make sense.
(In reply to Randy Lin [:rlin] from comment #2) > I would likely to change those things first. > 1. GetHeader -> GetMetaData( EncodeMetaData aMeta) EncodedMetaData > EncodeMetaData can be key-value or a structure, which used to store the meta > data from codec like > video:width, height, frameRate > audio:SampleFreq, bitDepth, Channels, codec_private_data > 2. GetEncodeTrack(nsTArray<unit8_t> aBuf, duration) -> > GetEncodeTrack(EncodePacket aPacket) GetEncodedTrack > EncodePacket for storing the audio/video encoded packet. video should has EncodedPacket > I/P frame information, timestamp, audio should has duration. > 3. WriteEncodeTrack(nsTArray<unit8_t> aBuf, duration) -> > WriteEncodeTrack(EncodePacket aPacket) WriteEncodedTrack > Same as point 2. I would suggest we remove the GetHeader(), and move the codec specific info or requirements into the GetEncodedTrack(), letting codec-encoder does its own implementation. Looks like it is hard to find a general term for such thing, and this suggestion has brought up by Rob before too. EncodedPacket sounds good to me, let's define the structure details later.
Assignee: nobody → rlin
Information need for video and audio, or subtitle in the future, track is different. Writer may need to 1. Query the track type from a Media track encoder object. 2. If it's an audio track encoder, query audio information. 3. If it's a video track, query video information Writer may need different audio/ video track information for specific codec, for example, it may need different META data from VP8 and WMV, how you handle this situation?
I'm thinking about this. In android, they design a hash table(KeyedVector) and let encoder to store the information. Key uses int32_t as type, The Value item they design another data structure to store various data type in it. Or has a big data structure which list all possible audio/video key-value.
Hopefully we won't need that. We shouldn't need to store metadata that we don't understand.
The MetaData part is used for exchange the header data from encoder /muxer, If we don't have to support many codecs yet, I may just try to Enumeration the possible data in single structure object.. Or have a flexibility data object to store those one. Or Add Get/Set XXX functions between encoder /muxer for MediaEncoder object to use.
No longer blocks: 879669
Three modals in my mind 1. XPCOM modal Use query interface between track encoder and container writer In ContainerWriter IVideoMETA *ivideo; trackEncoder->QueryInterface(IID_VideoMETA, (void **)&ivideo); trackEncoder->QueryInterface(IID_VPX, (void**)&ivp8); 2. AsXXX modal Refer to MediaStream::AsXXX RTTI modal IVideoMETA *ivideo = trackEncoder->AsVideoMETA(); IVPXMETA *ivpx = trackEncoder->AsVPXMETA(); 3. Have a huge structure which carry all info that container writer need. I don't suggest use this approach.
Randy, I can help to provide a WIP if you need
Good to hear that. If you have time, welcome to do this~
(In reply to C.J. Ku[:CJKu] from comment #9) > 3. Have a huge structure which carry all info that container writer need. I > don't suggest use this approach. Why not? How huge would this have to be?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12) > (In reply to C.J. Ku[:CJKu] from comment #9) > > 3. Have a huge structure which carry all info that container writer need. I > > don't suggest use this approach. > > Why not? How huge would this have to be? Size is not what I concern. Put all the data in a structure make every concrete track encoder class bundle with it, even if most of data in that structure have no meaning for that track encoder. I prefer #1 or #2 because the developer of concrete track encoder define the data it want to expose to muxer. For example, if I implement vorbis encoder, I want to export API for generic audio encoding META, such as sample rate, bits per sample, but I don't want to even see what opus want to expose to muxer, what opus specific data should be totally transparent to me. BTW, Randy, is there a table of META data that need to transmit from track to muxer?
Attached patch WIP v1 (obsolete) — — Splinter Review
This is the first WIP for this one, change the interface to carry audio or video encoded data or meta data.
Attachment #816236 - Flags: feedback?(slin)
Attachment #816236 - Flags: feedback?(roc)
Attachment #816236 - Flags: feedback?(cku)
As discussing with Randy, 1. EndOfStream needs to take a/v into consideration. (Only audio currently) 2. EndOfStream needs to check muxer if the last packet sends out (some file formats need to generate table after receiving EOS)
Muxer needs to accumulate enough audio/video data before generating output. It keeps several nsTArray<uint8_t> pointers for a while before muxing. So it probably needs a buffer manager to get/release a nsTArray<uint8_t> pointer, which will be used by encoder and muxer to avoid memory new/copy/release.
Comment on attachment 816236 [details] [diff] [review] WIP v1 Review of attachment 816236 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/MediaEncoder.cpp @@ +164,5 @@ > bool reloop = true; > while (reloop) { > switch (mState) { > case ENCODE_HEADER: { > + CommonMetaData *meta = mAudioEncoder->GetMetaData(); Would you like to move this whole part of setting up headers into the OpusTrackEncoder? It's a bit tricky here due to the flush flag and two kinds of headers, I can takeover the work if you don't mind. Everything else looks very good to me :)
Attachment #816236 - Flags: feedback?(slin) → feedback+
Comment on attachment 816236 [details] [diff] [review] WIP v1 Review of attachment 816236 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/CommonMetaData.h @@ +28,5 @@ > +public: > + virtual OpusMetaData* AsOpusMetaData() { return this; } > + virtual ~OpusMetaData() {} > + nsTArray<uint8_t> mHeader; > + nsTArray<uint8_t> mComment; Are these represent the two headers of Opus? If so, they should be mIdHeader and mCommentHeader. ::: content/media/encoder/TrackEncoder.h @@ +60,5 @@ > */ > + virtual nsresult GetEncodedTrack(EncodedData& aData) = 0; > + > + // This is used for exchange the metaData required for the muxer > + CommonMetaData* mMeta; Why not use the nsAutoPtr here? ::: content/media/ogg/OggWriter.h @@ +33,5 @@ > private: > nsresult Init(); > > + nsresult WriteEncodedTrack(const nsTArray<uint8_t>& aBuffer, int aDuration, > + uint32_t aFlags = 0); I think this is a bit confusing of having two methods with same names but different publicity, why not move the real work to the new one?
Comment on attachment 816236 [details] [diff] [review] WIP v1 Review of attachment 816236 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/CommonMetaData.h @@ +62,5 @@ > + uint32_t mSampleSize; > + uint32_t mFrameDuration; > +}; > + > +class OMXVideoMetaData: public CommonMetaData Please add references to documentation which defines the values in these classes. ::: content/media/encoder/EncodedData.h @@ +33,5 @@ > +}; > + > +// Represent one video encoded frame > + > +class EncodedVideoData : public EncodedData This is weird. What is EncodedVideoData::mAudio for? ::: content/media/encoder/TrackEncoder.h @@ +56,4 @@ > > /** > * Encodes raw segments. Result data is returned in aOutput. aOutputDuration > * is the playback duration of this packet in number of samples. Better fix this comment.
(In reply to Shelly Lin [:shelly] from comment #18) > Comment on attachment 816236 [details] [diff] [review] > WIP v1 > > Review of attachment 816236 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/encoder/MediaEncoder.cpp > @@ +164,5 @@ > > bool reloop = true; > > while (reloop) { > > switch (mState) { > > case ENCODE_HEADER: { > > + CommonMetaData *meta = mAudioEncoder->GetMetaData(); > > Would you like to move this whole part of setting up headers into the > OpusTrackEncoder? It's a bit tricky here due to the flush flag and two kinds > of headers, I can takeover the work if you don't mind. Everything else looks > very good to me :) We can change the state name from ENCODE_HEADER to be GET_METDATA and remove the flag. It seems used for opus encoder/ogg writer. I don't mind if you want to take over re-fine the opus/ogg part.
(In reply to Shelly Lin [:shelly] from comment #19) > Comment on attachment 816236 [details] [diff] [review] > WIP v1 > > Review of attachment 816236 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/encoder/CommonMetaData.h > @@ +28,5 @@ > > +public: > > + virtual OpusMetaData* AsOpusMetaData() { return this; } > > + virtual ~OpusMetaData() {} > > + nsTArray<uint8_t> mHeader; > > + nsTArray<uint8_t> mComment; > > Are these represent the two headers of Opus? If so, they should be mIdHeader > and mCommentHeader. OK. Change it. > > ::: content/media/encoder/TrackEncoder.h > @@ +60,5 @@ > > */ > > + virtual nsresult GetEncodedTrack(EncodedData& aData) = 0; > > + > > + // This is used for exchange the metaData required for the muxer > > + CommonMetaData* mMeta; > > Why not use the nsAutoPtr here? Good suggestion. > > ::: content/media/ogg/OggWriter.h > @@ +33,5 @@ > > private: > > nsresult Init(); > > > > + nsresult WriteEncodedTrack(const nsTArray<uint8_t>& aBuffer, int aDuration, > > + uint32_t aFlags = 0); > > I think this is a bit confusing of having two methods with same names but > different publicity, why not move the real work to the new one? hmm, I will try to re-fine this part. Thanks!
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > Comment on attachment 816236 [details] [diff] [review] > WIP v1 > > Review of attachment 816236 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/encoder/CommonMetaData.h > @@ +62,5 @@ > > + uint32_t mSampleSize; > > + uint32_t mFrameDuration; > > +}; > > + > > +class OMXVideoMetaData: public CommonMetaData > > Please add references to documentation which defines the values in these > classes. Sure~ > ::: content/media/encoder/EncodedData.h > @@ +33,5 @@ > > +}; > > + > > +// Represent one video encoded frame > > + > > +class EncodedVideoData : public EncodedData > > This is weird. What is EncodedVideoData::mAudio for? Oh..my mistake, fix it. > ::: content/media/encoder/TrackEncoder.h > @@ +56,4 @@ > > > > /** > > * Encodes raw segments. Result data is returned in aOutput. aOutputDuration > > * is the playback duration of this packet in number of samples. > > Better fix this comment. Sure. thanks.
Attached patch patch v2 (obsolete) — — Splinter Review
1. Add comment for the meta data/encoded data container and correct some in-proper comments. 3. EncodedVideoData::mAudio is used to store audio encoded data, so need to keep it. 4. Shelly, would you like to have a patch for removing the flush flag? Maybe need to pass the encoder status based by Comment 16.
Attachment #816236 - Attachment is obsolete: true
Attachment #816236 - Flags: feedback?(roc)
Attachment #816236 - Flags: feedback?(cku)
Attachment #817076 - Flags: review?(slin)
Attachment #817076 - Flags: review?(roc)
Comment on attachment 817076 [details] [diff] [review] patch v2 Review of attachment 817076 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/OpusTrackEncoder.cpp @@ +207,5 @@ > nsTArray<nsCString> comments; > comments.AppendElement(NS_LITERAL_CSTRING("ENCODER=Mozilla" MOZ_APP_UA_VERSION)); > > + SerializeOpusCommentHeader(vendor, comments, &mMeta->AsOpusMetaData()->mCommentHeader); > + mEncoderState = DATA; The id header has to be placed in the first page, and the comment header has to be placed in another separate page(s). OggWriter force generates a page by calling GetContainerData(buffers, ContainerWriter::FLUSH_NEEDED), so this is why we do the following in MediaEncoder: [for creating id header] OpusTrackEncoder sets up id header a buffer OggWriter writes this buffer ogg stream Get a ogg page from OggWriter with a flush flag Same deal with comment header. Btw, I don't think I can review your patch, please find a module owner or a module peer.
Attached patch patch v3 (obsolete) — — Splinter Review
Fix oggOpus header problem.
Attachment #817076 - Attachment is obsolete: true
Attachment #817076 - Flags: review?(slin)
Attachment #817076 - Flags: review?(roc)
Attachment #817730 - Flags: review?(roc)
Attachment #817730 - Flags: feedback?
Comment on attachment 817730 [details] [diff] [review] patch v3 Review of attachment 817730 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/ContainerWriter.h @@ +23,5 @@ > virtual ~ContainerWriter() {} > > enum { > + END_OF_STREAM = 1 << 0, > + GET_HEADER = 1 << 1 You pass this flag to GetContainerData(), so put it with the flag of FLUSH_NEEDED. @@ +41,3 @@ > > enum { > FLUSH_NEEDED = 1 << 0 Right here where I was talking about. @@ +54,5 @@ > virtual nsresult GetContainerData(nsTArray<nsTArray<uint8_t> >* aOutputBufs, > uint32_t aFlags = 0) = 0; > > protected: > + CommonMetaData *mMetaData; Use nsAutoPtr? ::: content/media/encoder/MediaEncoder.h @@ +50,5 @@ > class MediaEncoder : public MediaStreamListener > { > public : > enum { > + GET_METADDATA, I would name it ENCODE_METADATA, in order to parallel with others. ::: content/media/encoder/OpusTrackEncoder.cpp @@ +191,1 @@ > switch (mEncoderState) { Cool! So since you've merged the setup of id header and comment header into one call, I think we don't need the switch state here, and nor the mEncoderState. ::: content/media/ogg/OggWriter.cpp @@ +122,5 @@ > + if (aFlags & ContainerWriter::GET_HEADER) { > + nsresult rv = WriteEncodedData(mMetaData->AsOpusMetaData()->mIdHeader, 0); > + if (NS_FAILED(rv)) { > + return rv; > + } Try using NS_ENSURE_SUCCESS(rv, rv); @@ +123,5 @@ > + nsresult rv = WriteEncodedData(mMetaData->AsOpusMetaData()->mIdHeader, 0); > + if (NS_FAILED(rv)) { > + return rv; > + } > + rc = ogg_stream_flush(&mOggStreamState, &mOggPage); Add NS_ENSURE_TRUE(rc > 0, NS_ERROR_FAILURE); here. @@ +125,5 @@ > + return rv; > + } > + rc = ogg_stream_flush(&mOggStreamState, &mOggPage); > + ProduceOggPage(aOutputBufs); > + if (rc > 0) { No need to check rc here, plus you should check the rc right after where it is set. @@ +131,5 @@ > + if (NS_FAILED(rv)) { > + return rv; > + } > + rc = ogg_stream_flush(&mOggStreamState, &mOggPage); > + ProduceOggPage(aOutputBufs); Same here too. @@ +133,5 @@ > + } > + rc = ogg_stream_flush(&mOggStreamState, &mOggPage); > + ProduceOggPage(aOutputBufs); > + } > + return (rc > 0) ? NS_OK : NS_ERROR_FAILURE; I think you can return NS_OK; here. Just to make it clear that calling this function with GET_HEADER should always return something if everything goes well.
Comment on attachment 817730 [details] [diff] [review] patch v3 Review of attachment 817730 [details] [diff] [review]: ----------------------------------------------------------------- Basically looks OK ::: content/media/encoder/CommonMetaData.h @@ +27,5 @@ > +class OpusMetaData : public CommonMetaData > +{ > +public: > + virtual OpusMetaData* AsOpusMetaData() { return this; } > + virtual ~OpusMetaData() {} You don't need this destructor, it will be created implicitly. Same below. ::: content/media/encoder/ContainerWriter.h @@ +54,5 @@ > virtual nsresult GetContainerData(nsTArray<nsTArray<uint8_t> >* aOutputBufs, > uint32_t aFlags = 0) = 0; > > protected: > + CommonMetaData *mMetaData; Shouldn't mMetaData be per-track? ::: content/media/encoder/EncodedData.h @@ +26,5 @@ > + nsTArray<nsAutoPtr<EncodedAudioData> > mAudio; > +}; > + > +// Represent one video encoded frame > +class EncodedVideoData : public EncodedData I still don't understand why an EncodedVideoData needs an mAudio. Can you explain this in more detail? @@ +32,5 @@ > +public: > + enum FrameType { > + IFrame, > + PFrame, > + UNKNOW Please document these. ::: content/media/ogg/OggWriter.cpp @@ +119,5 @@ > { > int rc = -1; > + // Generate the oggOpus Header > + if (aFlags & ContainerWriter::GET_HEADER) { > + nsresult rv = WriteEncodedData(mMetaData->AsOpusMetaData()->mIdHeader, 0); I'd like to see a track type check somewhere as a guarantee that AsOpusMetaData will actually succeed. Also, "Metadata" (lower case d). ::: content/media/ogg/OggWriter.h @@ +27,5 @@ > > nsresult GetContainerData(nsTArray<nsTArray<uint8_t> >* aOutputBufs, > uint32_t aFlags = 0) MOZ_OVERRIDE; > > + void SetMetaData(CommonMetaData* aMetaData) { mMetaData = aMetaData->AsOpusMetaData(); } This AsOpusMetaData conversion is not required.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28) > Comment on attachment 817730 [details] [diff] [review] > patch v3 > > Review of attachment 817730 [details] [diff] [review]: > ----------------------------------------------------------------- > > Basically looks OK > > ::: content/media/encoder/CommonMetaData.h > @@ +27,5 @@ > > +class OpusMetaData : public CommonMetaData > > +{ > > +public: > > + virtual OpusMetaData* AsOpusMetaData() { return this; } > > + virtual ~OpusMetaData() {} > > You don't need this destructor, it will be created implicitly. Same below. Remove it, keep ~CommonMetaData() to avoid compiler complain "deleting object of polymorphic class type ‘mozilla::CommonMetaData’ which has non-virtual destructor might cause undefined behaviour" > ::: content/media/encoder/ContainerWriter.h > @@ +54,5 @@ > > virtual nsresult GetContainerData(nsTArray<nsTArray<uint8_t> >* aOutputBufs, > > uint32_t aFlags = 0) = 0; > > > > protected: > > + CommonMetaData *mMetaData; > > Shouldn't mMetaData be per-track? hmm, I haven’t think about this and would use nsTarray to hold this. > > ::: content/media/encoder/EncodedData.h > @@ +26,5 @@ > > + nsTArray<nsAutoPtr<EncodedAudioData> > mAudio; > > +}; > > + > > +// Represent one video encoded frame > > +class EncodedVideoData : public EncodedData > > I still don't understand why an EncodedVideoData needs an mAudio. Can you > explain this in more detail? The video track encoder only encode the video frame without audio data, I would like to change the mVideo name to mEncodedVideoFrames, mAudio to be mEncodedAudioFrames > > @@ +32,5 @@ > > +public: > > + enum FrameType { > > + IFrame, > > + PFrame, > > + UNKNOW > > Please document these. Sure. > ::: content/media/ogg/OggWriter.cpp > @@ +119,5 @@ > > { > > int rc = -1; > > + // Generate the oggOpus Header > > + if (aFlags & ContainerWriter::GET_HEADER) { > > + nsresult rv = WriteEncodedData(mMetaData->AsOpusMetaData()->mIdHeader, 0); > > I'd like to see a track type check somewhere as a guarantee that > AsOpusMetaData will actually succeed. humm, should add more check for get wrong type of mMetadata to avoid crash. > Also, "Metadata" (lower case d). > ::: content/media/ogg/OggWriter.h > @@ +27,5 @@ > > > > nsresult GetContainerData(nsTArray<nsTArray<uint8_t> >* aOutputBufs, > > uint32_t aFlags = 0) MOZ_OVERRIDE; > > > > + void SetMetaData(CommonMetaData* aMetaData) { mMetaData = aMetaData->AsOpusMetaData(); } > > This AsOpusMetaData conversion is not required. Remove the conversion.
(In reply to Shelly Lin [:shelly] from comment #27) > Comment on attachment 817730 [details] [diff] [review] > patch v3 > > Review of attachment 817730 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/encoder/ContainerWriter.h > @@ +23,5 @@ > > virtual ~ContainerWriter() {} > > > > enum { > > + END_OF_STREAM = 1 << 0, > > + GET_HEADER = 1 << 1 > > You pass this flag to GetContainerData(), so put it with the flag of > FLUSH_NEEDED. Now I know what has two enum... > @@ +41,3 @@ > > > > enum { > > FLUSH_NEEDED = 1 << 0 > > Right here where I was talking about. > > @@ +54,5 @@ > > virtual nsresult GetContainerData(nsTArray<nsTArray<uint8_t> >* aOutputBufs, > > uint32_t aFlags = 0) = 0; > > > > protected: > > + CommonMetaData *mMetaData; > > Use nsAutoPtr? Use AutoPtr would cause double free and crash...Because this pointer would auto recycle by encoder. > ::: content/media/encoder/MediaEncoder.h > @@ +50,5 @@ > > class MediaEncoder : public MediaStreamListener > > { > > public : > > enum { > > + GET_METADDATA, > > I would name it ENCODE_METADATA, in order to parallel with others. OK, change it to allign the naming. > ::: content/media/encoder/OpusTrackEncoder.cpp > @@ +191,1 @@ > > switch (mEncoderState) { > > Cool! So since you've merged the setup of id header and comment header into > one call, I think we don't need the switch state here, and nor the > mEncoderState. > > ::: content/media/ogg/OggWriter.cpp > @@ +122,5 @@ > > + if (aFlags & ContainerWriter::GET_HEADER) { > > + nsresult rv = WriteEncodedData(mMetaData->AsOpusMetaData()->mIdHeader, 0); > > + if (NS_FAILED(rv)) { > > + return rv; > > + } > > Try using NS_ENSURE_SUCCESS(rv, rv); > @@ +123,5 @@ > > + nsresult rv = WriteEncodedData(mMetaData->AsOpusMetaData()->mIdHeader, 0); > > + if (NS_FAILED(rv)) { > > + return rv; > > + } > > + rc = ogg_stream_flush(&mOggStreamState, &mOggPage); > > Add NS_ENSURE_TRUE(rc > 0, NS_ERROR_FAILURE); here. > > @@ +125,5 @@ > > + return rv; > > + } > > + rc = ogg_stream_flush(&mOggStreamState, &mOggPage); > > + ProduceOggPage(aOutputBufs); > > + if (rc > 0) { > > No need to check rc here, plus you should check the rc right after where it > is set. > > @@ +131,5 @@ > > + if (NS_FAILED(rv)) { > > + return rv; > > + } > > + rc = ogg_stream_flush(&mOggStreamState, &mOggPage); > > + ProduceOggPage(aOutputBufs); > > Same here too. > > @@ +133,5 @@ > > + } > > + rc = ogg_stream_flush(&mOggStreamState, &mOggPage); > > + ProduceOggPage(aOutputBufs); > > + } > > + return (rc > 0) ? NS_OK : NS_ERROR_FAILURE; > > I think you can return NS_OK; here. > Just to make it clear that calling this function with GET_HEADER should > always return something if everything goes well. Yes, good correction for those part and change it to proper one.
Attached patch patch v4 (obsolete) — — Splinter Review
I delay the multi-track support for this part. Maybe have another bug to implement this.
Attachment #817730 - Attachment is obsolete: true
Attachment #817730 - Flags: review?(roc)
Attachment #817730 - Flags: feedback?
Attachment #818227 - Flags: review?(roc)
Attachment #818227 - Flags: feedback?(slin)
Comment on attachment 818227 [details] [diff] [review] patch v4 Review of attachment 818227 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for taking my feedback, looks good overall! :) ::: content/media/encoder/MediaEncoder.cpp @@ +139,4 @@ > * Force copied the final container data from writer > * Return the copy of final container data > * Else > * Set mState to ENCODE_TRACK Nit: The common here is outdated due to the change. You can change it to: * Get the meta data from audio/video encoder * Write this meta data into the writer * Set mState to ENCODE_TRACK * Return the final container data ::: content/media/ogg/OggWriter.cpp @@ +50,4 @@ > uint32_t aFlags) > { > + for (uint32_t i = 0; i < aData.mEncodedAudioFrames.Length(); i++) { > + nsresult rv = WriteEncodedData(aData.mEncodedAudioFrames[i]->mData, aData.mEncodedAudioFrames[i]->mDuration, aFlags); Nit: Break this into 80-character line. @@ +129,5 @@ > + > + rc = ogg_stream_flush(&mOggStreamState, &mOggPage); > + NS_ENSURE_TRUE(rc > 0, NS_ERROR_FAILURE); > + ProduceOggPage(aOutputBufs); > + if (rc > 0) { Nit: You don't this this |if (rc > 0)| statement.
Attachment #818227 - Flags: feedback?(slin) → feedback+
Comment on attachment 818227 [details] [diff] [review] patch v4 Review of attachment 818227 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/CommonMetadata.h @@ +12,5 @@ > + > +class OpusMetadata; > +class VP8Metadata; > +class VorbisMetadata; > + Pre declare these classes: class VP8Metadata; class OpusMetadata; class VorbisMetadata; And move the definition of each concrete class to the header of each TrackEncoder, for exp, move OpusMetadata to OpusTrackEncoder.h Write should include this header, CommonMetadata.h, and the TrackEncoder header that it used. Couple Writer with the MetaData class definition it interest. ::: content/media/encoder/TrackEncoder.h @@ +54,2 @@ > */ > + virtual CommonMetadata* GetMetadata() = 0; If there is no perf concern, I would suggest clone a new Metadata object from mMeta, and return to caller. nsRefPtr<CommenMetadata> *GetMetadata() { nsRefPtr<CommonMetadata> data = mMeta.clone(); return data; } Since you export data member as public, it's better that you clone a meta and give it to caller, instead of return mMeta to that caller. And make CommonMetadata be reference base.
(In reply to C.J. Ku[:CJKu] from comment #33) > > Since you export data member as public, it's better that you clone a meta > and give it to caller, instead of return mMeta to that caller. You never know when will a users, Writer objects, change public properties and cause track encoder work abnormal.
Comment on attachment 818227 [details] [diff] [review] patch v4 Review of attachment 818227 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/ContainerWriter.h @@ +54,5 @@ > virtual nsresult GetContainerData(nsTArray<nsTArray<uint8_t> >* aOutputBufs, > uint32_t aFlags = 0) = 0; > > protected: > + CommonMetadata* mMetadata; make it nullptr in initial list(ContainerWriter::ContainerWriter()) ::: content/media/encoder/MediaEncoder.cpp @@ +164,5 @@ > bool reloop = true; > while (reloop) { > switch (mState) { > + case ENCODE_METADDATA: { > + CommonMetadata *meta = mAudioEncoder->GetMetadata(); It should never be nullptr. assertion instead of runtime null check
(In reply to Randy Lin [:rlin] from comment #29) > > ::: content/media/encoder/EncodedData.h > > @@ +26,5 @@ > > > + nsTArray<nsAutoPtr<EncodedAudioData> > mAudio; > > > +}; > > > + > > > +// Represent one video encoded frame > > > +class EncodedVideoData : public EncodedData > > > > I still don't understand why an EncodedVideoData needs an mAudio. Can you > > explain this in more detail? > The video track encoder only encode the video frame without audio data, I > would like to change the mVideo name to mEncodedVideoFrames, mAudio to be > mEncodedAudioFrames I don't understand this answer.
Comment on attachment 818227 [details] [diff] [review] patch v4 Review of attachment 818227 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/CommonMetadata.h @@ +39,5 @@ > +class VP8Metadata : public CommonMetadata > +{ > +public: > + // Set if the video is interlaced. > + uint32_t mFlagInterlaced; Should we declare data type here to align field size in container spec?
I removed it and will insert those on vp8TrackEncoder.h
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36) > (In reply to Randy Lin [:rlin] from comment #29) > > > ::: content/media/encoder/EncodedData.h > > > @@ +26,5 @@ > > > > + nsTArray<nsAutoPtr<EncodedAudioData> > mAudio; > > > > +}; > > > > + > > > > +// Represent one video encoded frame > > > > +class EncodedVideoData : public EncodedData > > > > > > I still don't understand why an EncodedVideoData needs an mAudio. Can you > > > explain this in more detail? > > The video track encoder only encode the video frame without audio data, I > > would like to change the mVideo name to mEncodedVideoFrames, mAudio to be > > mEncodedAudioFrames > > I don't understand this answer. hmm, It seems we can have a mEncodedVideo only and can contain videoframe(with audio) or audioFrame only encoded data? Is it what you mean? like this. +class EncodedData +{ +public: + // This container is used to store the video encoded packets. + // Muxer can check the length of both array and get the encoded data from those elements. + nsTArray<nsAutoPtr<EncodedVideoFrame> > mEncodedVideoFrames; +}; + +// Represent one video encoded frame +class EncodedVideoFrame : public EncodedData +{ +public: + enum VideoFrameType { + IFrame, // intraframe + PFrame, // predicted frame + BFrame, // bidirectionally predicted frame + UNKNOW // FrameType not set + }; + // Encoded video data + nsTArray<uint8_t> mVideoFrameData; + uint64_t mTimeStamp; + FrameType mVidoeFrameType; + + // Encoded audio data + nsTArray<uint8_t> mAudioFrameData; + // The playback duration of this packet in number of samples + uint64_t mDuration; +}; +}
(In reply to C.J. Ku[:CJKu] from comment #33) > Comment on attachment 818227 [details] [diff] [review] > patch v4 > > Review of attachment 818227 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/encoder/CommonMetadata.h > @@ +12,5 @@ > > + > > +class OpusMetadata; > > +class VP8Metadata; > > +class VorbisMetadata; > > + > > Pre declare these classes: > class VP8Metadata; > class OpusMetadata; > class VorbisMetadata; > > And move the definition of each concrete class to the header of each > TrackEncoder, for exp, move OpusMetadata to OpusTrackEncoder.h > > Write should include this header, CommonMetadata.h, and the TrackEncoder > header that it used. Couple Writer with the MetaData class definition it > interest. > > ::: content/media/encoder/TrackEncoder.h > @@ +54,2 @@ > > */ > > + virtual CommonMetadata* GetMetadata() = 0; > > If there is no perf concern, I would suggest clone a new Metadata object > from mMeta, and return to caller. > > nsRefPtr<CommenMetadata> *GetMetadata() > { > nsRefPtr<CommonMetadata> data = mMeta.clone(); For this one, I try to have a copy constructor on CommonMetadata, But this class can't see the real attribute (for ex opus's header/commnet), Is any better way to clone it? > return data; > } > > Since you export data member as public, it's better that you clone a meta > and give it to caller, instead of return mMeta to that caller. > > And make CommonMetadata be reference base. OK.
(In reply to Randy Lin [:rlin] from comment #40) > > > > nsRefPtr<CommenMetadata> *GetMetadata() > > { > > nsRefPtr<CommonMetadata> data = mMeta.clone(); > For this one, I try to have a copy constructor on CommonMetadata, But this > class can't see the real attribute (for ex opus's header/commnet), Is any > better way to clone it? > define a pure virtual function, named as clone, in CommonMetadata virtual nsRefPrt<CommonMetadata> Clone() = 0; And implement this function in each concrete CommonMetadata inherited. class VP8Metadata { nsRefPtr<CommonMetadata> Clone() { nsRefPrt<CommonMetada> data = new VP8Metadata(); // Copy all the attributes of this object into "data" return data; } }; Don't use copy consturctor or assign operator for this purpose.
Attached patch patch v5 (obsolete) — — Splinter Review
1. Allow store audio or video in EncodedFrame 2. nsRefPtr OpusMetadata 3. add clone method 4. Fix some nits
Attachment #818227 - Attachment is obsolete: true
Attachment #818227 - Flags: review?(roc)
Attachment #820414 - Flags: review?(roc)
Attachment #820414 - Flags: review?(cku)
Attached patch patch v5.1 (obsolete) — — Splinter Review
ah...change to this one, v5 can not build pass.
Attachment #820414 - Attachment is obsolete: true
Attachment #820414 - Flags: review?(roc)
Attachment #820414 - Flags: review?(cku)
Attachment #820771 - Flags: review?(cku)
Comment on attachment 820771 [details] [diff] [review] patch v5.1 Review of attachment 820771 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/OpusTrackEncoder.cpp @@ +224,2 @@ > > + return mMeta; 1. Check whether mMeta.get() is nullptr, if it is, create and setup; otherwise, just clone and return if (mMeta.get() == nullptr) { // Create and setup; } return mMeta->Clone(); // Don't return mMeta directly. 2. Make sure whether you need mMeta data member, or it can be a local variable in GetMetadata function. If mMeta member is not need, you don't need to clone, always create a new meta data in this function and return to the caller. ::: content/media/encoder/OpusTrackEncoder.h @@ +28,5 @@ > + nsRefPtr<CommonMetadata> Clone() > + { > + nsRefPtr<CommonMetadata> meta = new OpusMetadata(); > + meta->AsOpusMetadata()->mIdHeader = mIdHeader; > + meta->AsOpusMetadata()->mCommentHeader = mCommentHeader; nit-picking. Since all the other Clone function copy from here, we shouldmake it simpler. OpusMetadata *meta = new OpusMetadata(); // Clone META data. meta->mIdHeader = mIdHeader; meta->mCommentHeader = mCommentHeader; return meta; <-- Implicit conversion. ::: content/media/encoder/TrackEncoder.h @@ +59,5 @@ > */ > + virtual nsresult GetEncodedTrack(EncodedData& aData) = 0; > + > + // This is used for exchange the meta data required for the muxer > + nsRefPtr<CommonMetadata> mMeta; Make sure whether you need this data member. You can also create a new CommonMetadata in GetMetadata function. If the calling frequency of GetMetadata is low, only use it while handshaking between TrackEncoder and Writer, I will suggest just remove mMeta.
Attached patch patch v6 (obsolete) — — Splinter Review
1. remove AsXXXMetadata 2. remove mMetadata
Attachment #820771 - Attachment is obsolete: true
Attachment #820771 - Flags: review?(cku)
Attachment #820945 - Flags: review?(cku)
Comment on attachment 820945 [details] [diff] [review] patch v6 Review of attachment 820945 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/ogg/OggWriter.cpp @@ +127,5 @@ > { > int rc = -1; > + // Generate the oggOpus Header > + if (aFlags & ContainerWriter::GET_HEADER) { > + OpusMetadata* meta = static_cast<OpusMetadata*>(mMetadata.get()); Well, if you remove AsXXXMetadata function, RTTI mechanism, how you make sure the actual type of mMetadata is OpusMetadata instead of VorbisMatadata? you need RTTI here if (mMetadata->AsOpus()) { OpusMetadata *data = mMetadata->AsOpus(); // Put Opus data into container. } else if (mMetadata->AsVorbis()) { VorbisMetadata *data = mMetadata->AsOpus(); // Put Vorbis data into container. }
Comment on attachment 820945 [details] [diff] [review] patch v6 Review of attachment 820945 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/OpusTrackEncoder.cpp @@ +224,2 @@ > > + return meta->Clone(); Since meta is not a member data, you don't need to clone here. Return meta directly
Comment on attachment 820945 [details] [diff] [review] patch v6 Review of attachment 820945 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/ogg/OggWriter.h @@ +27,5 @@ > > nsresult GetContainerData(nsTArray<nsTArray<uint8_t> >* aOutputBufs, > uint32_t aFlags = 0) MOZ_OVERRIDE; > > + void SetMetadata(nsRefPtr<CommonMetadata> aMetadata) { mMetadata = aMetadata; } SetMetadata is called while start creating encoding pipeline. We may validate META data here, and return false if that METADATA is not what this writer accept For exp bool SetMetadata(....) { // Suppose OGG accetp Opus only if (aMetadata->AsOpus() == nullptr) return false; mMetaData = aMetadata; return true; } BTW, if we have video and audio track need to be multiplex in this writer, how to setup metadata for audio track and video track seperately?
Comment on attachment 820945 [details] [diff] [review] patch v6 Review of attachment 820945 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/ContainerWriter.h @@ +34,5 @@ > * Currently, WriteEncodedTrack doesn't support multiple tracks. > */ > + virtual nsresult WriteEncodedTrack(const EncodedData& aData, > + uint32_t aFlags = 0) = 0; > + enum TrackType { AUDIO, VIDEO /*SUBTITLE in the future*/ }; virtual nsresult WriteEncodedTrack(TrackType type, const nsTArray<uint8_t>& aBuffer, int aDuration, uint32_t aFlags = 0); In the future, after we support multiple tracks, we may add one more paremeter. virtual nsresult WriteEncodedTrack(TrackType type, unsigned_int trackIndex, const nsTArray<uint8_t>& aBuffer, int aDuration, uint32_t aFlags = 0);
(In reply to C.J. Ku[:CJKu] from comment #47) > Comment on attachment 820945 [details] [diff] [review] > patch v6 > > Review of attachment 820945 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/encoder/OpusTrackEncoder.cpp > @@ +224,2 @@ > > > > + return meta->Clone(); > > Since meta is not a member data, you don't need to clone here. Return meta > directly So.....Do we need Clone?
(In reply to Randy Lin [:rlin] from comment #50) > (In reply to C.J. Ku[:CJKu] from comment #47) > > Comment on attachment 820945 [details] [diff] [review] > > patch v6 > > > > Review of attachment 820945 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: content/media/encoder/OpusTrackEncoder.cpp > > @@ +224,2 @@ > > > > > > + return meta->Clone(); > > > > Since meta is not a member data, you don't need to clone here. Return meta > > directly > > So.....Do we need Clone? After you remove mMeta in TrackEncoder, there is no need for Clone anymore.
(In reply to C.J. Ku[:CJKu] from comment #48) > BTW, if we have video and audio track need to be multiplex in this writer, > how to setup metadata for audio track and video track seperately? Change void SetMetadata to bool SetMetadata(TrackType aType, nsRefPtr<CommonMetadata> aData) Check aData in each writer's SetMetadata, return false if aData is not acceptable because of 1. Type not match: such as put a VP8 video track into OGG container. 2. properties of aData is not valid: wrong sample rate, for exp.
(In reply to C.J. Ku[:CJKu] from comment #47) > Comment on attachment 820945 [details] [diff] [review] > patch v6 > > Review of attachment 820945 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/encoder/OpusTrackEncoder.cpp > @@ +224,2 @@ > > > > + return meta->Clone(); > > Since meta is not a member data, you don't need to clone here. Return meta > directly So.....Do we need Clone?(In reply to C.J. Ku[:CJKu] from comment #49) > Comment on attachment 820945 [details] [diff] [review] > patch v6 > > Review of attachment 820945 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/encoder/ContainerWriter.h > @@ +34,5 @@ > > * Currently, WriteEncodedTrack doesn't support multiple tracks. > > */ > > + virtual nsresult WriteEncodedTrack(const EncodedData& aData, > > + uint32_t aFlags = 0) = 0; > > + > > enum TrackType { > AUDIO, > VIDEO /*SUBTITLE in the future*/ > }; > > virtual nsresult WriteEncodedTrack(TrackType type, const nsTArray<uint8_t>& > aBuffer, > int aDuration, uint32_t aFlags = 0); > > In the future, after we support multiple tracks, we may add one more > paremeter. > virtual nsresult WriteEncodedTrack(TrackType type, unsigned_int trackIndex, > const nsTArray<uint8_t>& aBuffer, > int aDuration, uint32_t aFlags = 0); EncodedFrame has this and it can tell muxer what's in it. + FrameType mFrameType;
(In reply to C.J. Ku[:CJKu] from comment #52) > (In reply to C.J. Ku[:CJKu] from comment #48) > > BTW, if we have video and audio track need to be multiplex in this writer, > > how to setup metadata for audio track and video track seperately? > > Change > void SetMetadata > to > bool SetMetadata(TrackType aType, nsRefPtr<CommonMetadata> aData) > > Check aData in each writer's SetMetadata, return false if aData is not > acceptable because of > 1. Type not match: such as put a VP8 video track into OGG container. > 2. properties of aData is not valid: wrong sample rate, for exp. Can I put the TrackType in CommonMetadata?
(In reply to Randy Lin [:rlin] from comment #54) > (In reply to C.J. Ku[:CJKu] from comment #52) > > (In reply to C.J. Ku[:CJKu] from comment #48) > > > BTW, if we have video and audio track need to be multiplex in this writer, > > > how to setup metadata for audio track and video track seperately? > > > > Change > > void SetMetadata > > to > > bool SetMetadata(TrackType aType, nsRefPtr<CommonMetadata> aData) > > > > Check aData in each writer's SetMetadata, return false if aData is not > > acceptable because of > > 1. Type not match: such as put a VP8 video track into OGG container. > > 2. properties of aData is not valid: wrong sample rate, for exp. > > Can I put the TrackType in CommonMetadata? You can use AsXXX to tell the type of track. In this case, you still need change Writer::mMeta to a list or multiple nsRefPrt<CommonMetadata> for different track.
suggest to rename CommonMetadata as TrackMetadataBase
Attached patch patch v7 (obsolete) — — Splinter Review
Attachment #820945 - Attachment is obsolete: true
Attachment #820945 - Flags: review?(cku)
Attachment #821057 - Flags: review?(cku)
Comment on attachment 821057 [details] [diff] [review] patch v7 Review of attachment 821057 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/CommonMetadata.h @@ +13,5 @@ > +// A class represent meta data for various codec format. Only support one track information. > +class CommonMetadata > +{ > +public: > + enum MetadataKind { Suggestion:rename to TrackType ::: content/media/encoder/EncodedData.h @@ +22,5 @@ > +public: > + // This container is used to store the video or audio encoded packets. > + // Muxer should check mFrameType and get the encoded data type from mEncodedFrames. > + nsTArray<nsAutoPtr<EncodedFrame> > mEncodedFrames; > +}; You plan to put audio EncodedFrame and video EncodedFrame into a EncodedData container in MediaEncoder, and send this EncodedData into Writer? Don't public mEncodedFrames. class EncodedData { public: void addEncodedFrame(); int getEncodedFrameCount() const; EncodedFrame &getEncodedFrame() const; private: nsTArray<...; }; So that Writer can only get EncodedFrame of this container, and TrackEncoder can add Encoded frame into this container, @@ +41,5 @@ > + uint64_t mTimeStamp; > + // The playback duration of this packet in number of samples > + uint64_t mDuration; > + // Represet what is in the FrameData > + FrameType mFrameType; Don't public these data, add getter/setter functions. And initiate all data members in constructor. ::: content/media/encoder/EncodedFrame.h @@ +8,5 @@ > + > +#include "nsTArray.h" > + > +namespace mozilla { > + EncodedFrame.h and EncodedData.h are duplicated
(In reply to C.J. Ku[:CJKu] from comment #58) > Comment on attachment 821057 [details] [diff] [review] > patch v7 > > Review of attachment 821057 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/encoder/CommonMetadata.h > @@ +13,5 @@ > > +// A class represent meta data for various codec format. Only support one track information. > > +class CommonMetadata > > +{ > > +public: > > + enum MetadataKind { > > Suggestion:rename to TrackType make sense. > > ::: content/media/encoder/EncodedData.h > @@ +22,5 @@ > > +public: > > + // This container is used to store the video or audio encoded packets. > > + // Muxer should check mFrameType and get the encoded data type from mEncodedFrames. > > + nsTArray<nsAutoPtr<EncodedFrame> > mEncodedFrames; > > +}; > > You plan to put audio EncodedFrame and video EncodedFrame into a EncodedData > container in MediaEncoder, and send this EncodedData into Writer? No...It is for video I/P frame. > Don't public mEncodedFrames. > class EncodedData { > public: > void addEncodedFrame(); > int getEncodedFrameCount() const; > EncodedFrame &getEncodedFrame() const; > > private: > nsTArray<...; > }; > > So that Writer can only get EncodedFrame of this container, and TrackEncoder > can add Encoded frame into this container, OK, protect the data member. > @@ +41,5 @@ > > + uint64_t mTimeStamp; > > + // The playback duration of this packet in number of samples > > + uint64_t mDuration; > > + // Represet what is in the FrameData > > + FrameType mFrameType; > > Don't public these data, add getter/setter functions. And initiate all data > members in constructor. OK. hide it. > ::: content/media/encoder/EncodedFrame.h > @@ +8,5 @@ > > + > > +#include "nsTArray.h" > > + > > +namespace mozilla { > > + > > EncodedFrame.h and EncodedData.h are duplicated Let me check check...
Comment on attachment 820771 [details] [diff] [review] patch v5.1 Review of attachment 820771 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/CommonMetadata.h @@ +14,5 @@ > +// A class represent meta data for various codec format. Only support one track information. > +class CommonMetadata > +{ > +public: > + virtual OpusMetadata* AsOpusMetadata() { return nullptr; } You can use a template function to do the conversion to avoid function proliferating when more codec formats are supported. @@ +17,5 @@ > +public: > + virtual OpusMetadata* AsOpusMetadata() { return nullptr; } > + virtual ~CommonMetadata() {} > + virtual nsrefcnt AddRef() = 0; > + virtual nsrefcnt Release() = 0; Any reason for ref-counting functions to be virtual? You can say NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CommonMetadata) here so that sub-classes won't have to repeat it on their own. ::: content/media/encoder/EncodedData.h @@ +22,5 @@ > +public: > + // This container is used to store the video or audio encoded packets. > + // Muxer should check mFrameType and get the encoded data type from mEncodedFrames. > + nsTArray<nsAutoPtr<EncodedFrame> > mEncodedFrames; > +}; Is EncodedData worth a new type or will it have new members in the future? If not, you can just say typedef nsTArray<nsAutoPtr<EncodedFrame> > EncodedData. ::: content/media/encoder/OpusTrackEncoder.cpp @@ +340,3 @@ > // result is returned as opus error code if it is negative. > int result = 0; > #ifdef MOZ_SAMPLE_TYPE_S16 Should audiodata->mFrameData.Elements also be passed to opus_encode() in this code path?
Attachment #820771 - Attachment is obsolete: false
(In reply to Randy Lin [:rlin] from comment #59) > > ::: content/media/encoder/EncodedData.h > > @@ +22,5 @@ > > > +public: > > > + // This container is used to store the video or audio encoded packets. > > > + // Muxer should check mFrameType and get the encoded data type from mEncodedFrames. > > > + nsTArray<nsAutoPtr<EncodedFrame> > mEncodedFrames; > > > +}; > > > > You plan to put audio EncodedFrame and video EncodedFrame into a EncodedData > > container in MediaEncoder, and send this EncodedData into Writer? > No...It is for video I/P frame. > I see. Then I have a question here. Raw audio/ video samples synchronization is done in MediaStreamGraph already(correct me if I was wrong). MediaEncoder pull encoded data from TrackEncoder, by GetEncodedTrack, and write into ContainerWriter, by WriteEncodedTrack. If the duration of "AudioTrackEncoder::GetEncodedTrack +VideoTrackEncoder::GetEncodedTrack" is always longer then MediaRecorder::Session::mTimeSlice, then we may need 2 hours, for exp, to record one hour recording session. Do we have an mechanism to fix this problem? Such as random source sample drop in MediaEncoder?
(In reply to C.J. Ku[:CJKu] from comment #61) > (In reply to Randy Lin [:rlin] from comment #59) > > > ::: content/media/encoder/EncodedData.h > > > @@ +22,5 @@ > > > > +public: > > > > + // This container is used to store the video or audio encoded packets. > > > > + // Muxer should check mFrameType and get the encoded data type from mEncodedFrames. > > > > + nsTArray<nsAutoPtr<EncodedFrame> > mEncodedFrames; > > > > +}; > > > > > > You plan to put audio EncodedFrame and video EncodedFrame into a EncodedData > > > container in MediaEncoder, and send this EncodedData into Writer? > > No...It is for video I/P frame. > > > I see. Then I have a question here. > Raw audio/ video samples synchronization is done in MediaStreamGraph > already(correct me if I was wrong). > MediaEncoder pull encoded data from TrackEncoder, by GetEncodedTrack, and > write into ContainerWriter, by WriteEncodedTrack. If the duration of > "AudioTrackEncoder::GetEncodedTrack +VideoTrackEncoder::GetEncodedTrack" is > always longer then MediaRecorder::Session::mTimeSlice, then we may need 2 > hours, for exp, to record one hour recording session. > > Do we have an mechanism to fix this problem? Such as random source sample > drop in MediaEncoder? mTimeSlice is just a timer alike mechanism and used to generate blob data and not related this question. For the encoder performance problem, we may adjust lower bitrate/quality parameter to meet the fps requirement.
Attached patch patch v8 (obsolete) — — Splinter Review
1.renmae CommonMetadata 2.protect encodeddata member 3.fix some nits
Attachment #820771 - Attachment is obsolete: true
Attachment #821057 - Attachment is obsolete: true
Attachment #821057 - Flags: review?(cku)
Attachment #821602 - Flags: review?(cku)
Comment on attachment 821602 [details] [diff] [review] patch v8 Review of attachment 821602 [details] [diff] [review]: ----------------------------------------------------------------- overall, look good to me. ::: content/media/encoder/EncodedData.h @@ +16,5 @@ > + * This container is used to carry video or audio encoded data from encoder to muxer. > + * The media data object is created by encoder and recycle by the destructor. > + * Only allow to store audio or video encoded data in EncodedData. > + */ > +class EncodedData Rename to EncodedChunk or EncodedFrames or EncodedFrameContainer. It's more semantically match. EncodedData is too generic @@ +21,5 @@ > +{ > +public: > + // Append encoded frame data > + void AppendEncodedFrame(EncodedFrame* aEncodedFrame) > + { Have assertion to make sure AppendEncodedFrame &GetEncodedFrames are called in the same thread(encode thread) @@ +54,5 @@ > + void SetFrameData(nsTArray<uint8_t> *aData) > + { > + mFrameData.SwapElements(*aData); > + } > + const uint64_t GetTimeStamp() { return mTimeStamp; } uint64_t GetTimeStamp() const @@ +57,5 @@ > + } > + const uint64_t GetTimeStamp() { return mTimeStamp; } > + void SetTimeStamp(uint64_t aTimeStamp) { mTimeStamp = aTimeStamp; } > + > + const uint64_t GetDuration() { return mDuration; } uint64_t GetDuration() const @@ +60,5 @@ > + > + const uint64_t GetDuration() { return mDuration; } > + void SetDuration(uint64_t aDuration) { mDuration = aDuration; } > + > + const FrameType GetFrameType() { return mFrameType; } FrameType GetFrameType() const @@ +62,5 @@ > + void SetDuration(uint64_t aDuration) { mDuration = aDuration; } > + > + const FrameType GetFrameType() { return mFrameType; } > + void SetFrameType(FrameType aFrameType) { mFrameType = aFrameType; } > +private: Have a constructor to initialize mTimeStamp, mDuration, mFrameType(UNKNOW) ::: content/media/encoder/TrackMetadataBase.h @@ +21,5 @@ > + METADATA_UNKNOW // Metadata Kind not set > + }; > + virtual ~TrackMetadataBase() {} > + // Store the kind of meta data in this object and let muxer can identify which type in this object. > + MetadataKind mMetadataKind; Remove this member data and add a abstract function in base class virtual MetadataKind GetKind() const = 0; Define concrete function in each derived class OpusMetadata: public TrackMetadataBase { public: MetadataKind GetKind() const MOZ_OVERRIDE { return METADATA_OPUS; } }; ::: content/media/ogg/OggWriter.cpp @@ +168,5 @@ > } > > +nsresult > +OggWriter::SetMetadata(nsRefPtr<TrackMetadataBase> aMetadata) > +{ Comment // Metadata type checking. Reject unacceptable track encoder @@ +172,5 @@ > +{ > + if (aMetadata->mMetadataKind != TrackMetadataBase::METADATA_OPUS) { > + LOG("wrong meta data type!"); > + return NS_ERROR_FAILURE; > + } Comment // Validate each field of METADATA
Attached patch patch v9 (obsolete) — — Splinter Review
Fix nits, change naming to EncodedFrameContainer.h Skip threading checking on this patch.
Attachment #821602 - Attachment is obsolete: true
Attachment #821602 - Flags: review?(cku)
Attachment #821684 - Flags: review?(roc)
Attachment #821684 - Flags: review?(cku)
Attachment #821684 - Flags: review?(cku) → review+
Attached patch patch v9.1 (obsolete) — — Splinter Review
Carry cjku as reviewer, Fix try server build break, and we still needs roc to review it. https://tbpl.mozilla.org/?tree=Try&rev=8093764baa8d
Attachment #821684 - Attachment is obsolete: true
Attachment #821684 - Flags: review?(roc)
Attachment #822098 - Flags: review?(roc)
Comment on attachment 822098 [details] [diff] [review] patch v9.1 Review of attachment 822098 [details] [diff] [review]: ----------------------------------------------------------------- much better :-)
Attachment #822098 - Flags: review?(roc) → review+
Attached patch check-in patch — — Splinter Review
Thanks all for reviewing it, carry reviewer, check-in needed.
Attachment #822098 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0c1d4dbe85c9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 935774
Depends on: 936352
Component: Video/Audio → Video/Audio: Recording
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: