Closed Bug 891704 Opened 12 years ago Closed 11 years ago

[MediaEncoder] Implement MP4Writer

Categories

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

Product:
Core
Component:
Audio/Video: Recording
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
1.3 C2/1.4 S2(17jan)
Project Flags:
blocking-b2g 1.4+
Tracking Flags:
Tracking Status
firefox28 --- wontfix
firefox29 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: u459114, Assigned: ayang)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [ft:multimedia-platform])

Attachments

(1 file, 29 obsolete files)

fmp4_muxer
96.55 KB, patch
ayang
: review+
Details | Diff | Splinter Review
Receive audio/video track encoded data and multiplex into MP4 container. Receive audio track encode data and put audio only data into MP4 container.
Blocks: MediaEncoder
Assignee: nobody → ayang
Depends on: 905582
Blocks: b2g-multimedia
No longer blocks: b2g-multimedia
Attached patch fragment_mp4_muxer (obsolete) — — Splinter Review
a pretty draft version. 1. refactory box definition from header to cpp implementation? 2. codecSpecificConfig between muxer and encoder? esds box, avcC box. 3. keep audio/video track concept? 4. refactory muxerin each box to a pure writer interface? 5. refactory 2-pass muxing to a better understrand way? 6. how to test this part whitout encoder? c++ standalone unit test seems very difficult in gecko. 7. meta data structure between omx encoder needs to be defined. 8. integrate to MediaRecorder. 9. refactory to a better state-machine in muxer when integrated into MediaRecorder.
10. a better way to separate this patch into several patches to different bugs?
blocking-b2g: --- → 1.3+
Whiteboard: [ft:media-recording]
No longer blocks: 879669
Depends on: 879669
Blocks: 923038
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Attached patch fragment_mp4_muxer (obsolete) — — Splinter Review
WIP
Attachment #821548 - Attachment is obsolete: true
Attachment #827331 - Attachment is patch: true
Attached patch fragment_mp4_muxer (obsolete) — — Splinter Review
wip version 2, integrated to ContainerWriter and add a mock audio track encoder
Attachment #827331 - Attachment is obsolete: true
Attached patch test (obsolete) — — Splinter Review
mock audio track encoder
Attached patch fragment_mp4_muxer (obsolete) — — Splinter Review
Move to content/media/fmp4/muxer and add comments.
Attachment #829194 - Attachment is obsolete: true
Attachment #829196 - Attachment is obsolete: true
Attached patch iso_media_muxer (obsolete) — — Splinter Review
wip, aac mp4 muxer
Attachment #830751 - Attachment is obsolete: true
Comment on attachment 8335076 [details] [diff] [review] iso_media_muxer Review of attachment 8335076 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOCompositor.cpp @@ +13,5 @@ > +namespace mozilla { > + > +#define BUFFER_SIZE 1024*1024 > + > +Fragmentation::Fragmentation(uint32_t aTrackType, ISOCompositor* aCompositor) Change the second parameter to Fragmentation::Fragmentation(uint32_t aTrackType, MetaData) to decouple Fragmentaion with ISOCompositor ::: content/media/fmp4/muxer/ISOCompositor.h @@ +66,5 @@ > + > +/** > + * ISOCompositor acts 3 different roles: > + * 1. holds the information about audio metadata, video metadata, > + * current time, fragmentation, and generating stream data. It does not need to hold metadata. Metadata context can should in MP4Writer and share to all objects which need it, such as ISOCompositor or Fragemenation @@ +92,5 @@ > + uint32_t Write(uint8_t* aBuf, uint32_t aSize); > + uint32_t WriteInt8(uint8_t aInt8); > + uint32_t WriteInt16(uint16_t aInt16); > + uint32_t WriteInt32(uint32_t aInt32); > + uint32_t WriteInt64(uint64_t aInt64); I would suggest use template member function here ::: content/media/fmp4/muxer/ISOMediaWriter.cpp @@ +24,5 @@ > + , mState(MUXING_HEAD) > + , mBlobReady(false) > +{ > + mCompositor = new ISOCompositor(); > + mCompositor->SetFragmentDuration(FRAG_DURATION); Suggest move these two lines into Init function @@ +149,5 @@ > + mCompositor->SetFragment(mVidFrag); > + return NS_OK; > + } > + > + LOG("wrong meta data type!"); Add an ASSERTION here to make it crash in debug version
Comment on attachment 8335076 [details] [diff] [review] iso_media_muxer Review of attachment 8335076 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOMediaBoxes.h @@ +12,5 @@ > +#include "nsAutoPtr.h" > +#include "MuxerOperation.h" > +#include "MP4ESDS.h" > + > +using namespace std; Don't use "using namespace" in a header file. It will import everything in the namespace into the compilation unit, and that may cause unexpected errors in other users of the header. Always prefix with the namespace in headers, i.e. std::bitset. @@ +127,5 @@ > + > +class TrackRunBox : public FullBox { > +public: > + // flags for TrackRunBox::flags. > + const static uint32_t flags_data_offset_present = 0x000001; Isn't initializing statics like this a C++11x feature? We may need to support older compilers, like Visual Studio 2010, so this needs to be changed to an enum, #defines, or the values need to be initialized in the cpp file. ::: content/media/fmp4/muxer/ISOMediaWriter.h @@ +37,5 @@ > + * MUXING_DONE: > + * It generates the mfra as the last blob. > + */ > + enum { > + MUXING_HEAD, I recommend you name this enum type, say MuxingState, and have mState below an instance of that type.
Attached patch 1_iso_media_compositor (obsolete) — — Splinter Review
Attachment #8335076 - Attachment is obsolete: true
Attached patch 2_iso_media_box (obsolete) — — Splinter Review
Attached patch 3_iso_media_writer (obsolete) — — Splinter Review
Divide patch
(In reply to Chris Pearce (:cpearce) from comment #9) > @@ +37,5 @@ > > + * MUXING_DONE: > > + * It generates the mfra as the last blob. > > + */ > > + enum { > > + MUXING_HEAD, > > I recommend you name this enum type, say MuxingState, and have mState below > an instance of that type. Thanks for comment, all are fixed.
Comment on attachment 8335751 [details] [diff] [review] 3_iso_media_writer Review of attachment 8335751 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOMediaWriter.cpp @@ +24,5 @@ > + , mState(MUXING_HEAD) > + , mBlobReady(false) > +{ > + mCompositor = new ISOCompositor(); > + mCompositor->SetFragmentDuration(FRAG_DURATION); All the member data here is not thread safe. Thing works because you assume the caller will call all public function in the same thread and it does. This assumption is dangerous, since caller has no idea on that. With re-factory in the future, this assumption might be failed Record thread ID in constructor and make assertion in all public functions that all these functions are runs in the same thread @@ +28,5 @@ > + mCompositor->SetFragmentDuration(FRAG_DURATION); > +} > + > +nsresult > +ISOMediaWriter::RunState(uint32_t aTrackType) Rename to ComposeByState? @@ +83,5 @@ > + } > + > + frag->AddFrame(frame); > + } > + Move 72 ~ 83 to another function, e.g. GetFragmentByType() Code here change to // Push frames into correct Fragment uint32_t len = aData.GetEncodedFrames().Length(); for (uint32_t i = 0; i < len; i++) { nsRefPtr<EncodedFrame> frame(aData.GetEncodedFrames()[i]); Fragmentation *frag = GetFragmentByType(frame); MOZ_ASSERT(frag.get()); frag->AddFrame(frame); } @@ +92,5 @@ > + } > + > + nsresult rv; > + if (mCompositor->HasAudioTrack() && > + (mAudFrag->HasEnoughData() || mAudFrag->EOS())) { For easy to read, change if statement here to if (mCompositor->HasAudioTrack()) { if (mAudFrag->HasEnoughData() || mAudFrag->EOS()) { rv = RunState(Audio_Track); NS_ENSURE_SUCCESS(rv, rv); } } @@ +123,5 @@ > +{ > + if (mBlobReady) { > + mBlobReady = false; > + aOutputBufs->AppendElement(); > + return mCompositor->GetBuf(aOutputBufs->LastElement()); mCompositor->GetBuf to a temp buffer and append it to aOutputBufs only if mCompositor->GetBuf return success. @@ +152,5 @@ > + aMetadata); > + mCompositor->SetFragment(mVidFrag); > + return NS_OK; > + } > + Change if statements here to switch is better for extendability. switch (aMetadata->GetKind()) { case TrackMetadataBase::METADATA_AAC: ... break; case TrackMetadataBase::METADATA_AVC: ... break; default: LOG("wrong meta data type!"); MOZ_ASSERT(false); return NS_ERROR_FAILURE; } } ::: content/media/fmp4/muxer/ISOMediaWriter.h @@ +12,5 @@ > + > +class ISOCompositor; > +class Fragmentation; > +class AACTrackMetadata; > +class AVCTrackMetadata; There is no need to predeclare these two class here class AACTrackMetadata; class AVCTrackMetadata; @@ +41,5 @@ > + MUXING_HEAD, > + MUXING_FRAG, > + MUXING_DONE, > + }; > + Need to be public? I think this enum is for internal only
Comment on attachment 8335749 [details] [diff] [review] 1_iso_media_compositor Review of attachment 8335749 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOCompositor.cpp @@ +11,5 @@ > +using mozilla::NativeEndian; > + > +namespace mozilla { > + > +#define BUFFER_SIZE 512*1024 magic number? :) @@ +22,5 @@ > + , mEOS(false) > +{ > + > + if (mTrackType == Audio_Track) { > + nsRefPtr<AACTrackMetadata> audMeta = static_cast<AACTrackMetadata*>(aMetadata); need more check for audMeta? @@ +23,5 @@ > +{ > + > + if (mTrackType == Audio_Track) { > + nsRefPtr<AACTrackMetadata> audMeta = static_cast<AACTrackMetadata*>(aMetadata); > + mSamplePerFragment = mFragDuration * audMeta->SampleRate / audMeta->FrameDuration; zero div? @@ +25,5 @@ > + if (mTrackType == Audio_Track) { > + nsRefPtr<AACTrackMetadata> audMeta = static_cast<AACTrackMetadata*>(aMetadata); > + mSamplePerFragment = mFragDuration * audMeta->SampleRate / audMeta->FrameDuration; > + } else { > + nsRefPtr<AVCTrackMetadata> vidMeta = static_cast<AVCTrackMetadata*>(aMetadata); The same as above. @@ +98,5 @@ > +ISOCompositor::ISOCompositor() > + : mAudFrag(nullptr) > + , mVidFrag(nullptr) > + , mFragNum(0) > + , mFragDuration(10) 10 is ? @@ +143,5 @@ > + return NS_ERROR_FAILURE; > +} > + > +nsresult > +ISOCompositor::GetAudMetadata(AACTrackMetadata** aAudMeta) GetAudioMetadata.. @@ +181,5 @@ > +bool > +ISOCompositor::HasVideoTrack() > +{ > + nsRefPtr<AVCTrackMetadata> vidMeta; > + GetVidMetadata(getter_AddRefs(vidMeta)); GetVideoMetadata ::: content/media/fmp4/muxer/ISOCompositor.h @@ +41,5 @@ > + uint32_t GetSampleNumberPerFragment(); > + uint32_t GetType() { return mTrackType; } > + bool HasEnoughData(); > + bool EOS() { return mEOS; } > + nsresult Flush(); function/variable mixing @@ +60,5 @@ > + // on codec type. > + nsRefPtr<EncodedFrame> mCSDFrame; > + > + // END_OF_STREAM from ContainerWriter > + bool mEOS; sort by variable size.. @@ +106,5 @@ > + > + // This is called by GetContainerData and swap the buffer to aOutBuf. > + nsresult GetBuf(nsTArray<uint8_t>& aOutBuf); > + > + uint32_t GetTime(); What's time? @@ +137,5 @@ > + uint32_t GetBufPos() { return mOutBuffer.Length(); } > + nsresult FlushBuf(); > + > +protected: > + Fragmentation* mAudFrag; mAudioFrag @@ +138,5 @@ > + nsresult FlushBuf(); > + > +protected: > + Fragmentation* mAudFrag; > + Fragmentation* mVidFrag; mVideoFrag @@ +161,5 @@ > + uint64_t mOutputSize; > + > + // bit writing operation. Note: the mBitCount should be 0 before any > + // byte-boundary writting method (WriteInt8(), for instance) be called. > + uint8_t mBitCount; Used for remember how many bits have been written to buffer? To avoid un-align to byte? ::: content/media/fmp4/muxer/ISOTrackMetadata.h @@ +46,5 @@ > + uint32_t AvgBitrate; > + uint32_t SampleRate; // From 14496-3 table 1.16, it could be 7350 ~ 96000. > + uint32_t FrameDuration; // Audio frame duration based on SampleRate. > + uint32_t FrameSize; // Audio frame size, 0 is variant size. > + uint32_t Channels; // Channel number, it should be 1 or 2. sort by member size
Comment on attachment 8335749 [details] [diff] [review] 1_iso_media_compositor Review of attachment 8335749 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOCompositor.h @@ +97,5 @@ > + uint32_t WriteInt16(uint16_t aInt16); > + uint32_t WriteInt32(uint32_t aInt32); > + uint32_t WriteInt64(uint64_t aInt64); > + uint32_t WriteInt16Array(uint16_t *aInt16Array, uint32_t aSize); > + uint32_t WriteInt32Array(uint32_t *aInt32Array, uint32_t aSize); template <typename T> uint32_t Write(T aData); template <typename T> uint32_t WriteArray(const T &aArray, uint32_t aSize); WriteFourCC is too specific. I would suggest just call WriteArray<byte>(aString, 4) for the same purpose @@ +164,5 @@ > + // byte-boundary writting method (WriteInt8(), for instance) be called. > + uint8_t mBitCount; > + uint8_t mBit; > +}; > + template <typename T> uint32_t ISOCompositor::Write(T aData) { MOZ_ASSERT(!mBitCount); aData = NativeEndian::swapToNetworkOrder(aData); Write((uint8_t*)&aData, sizeof(T)); return sizeof(T); } template <typename T> uint32_t ISOCompositor::WriteArray(const T &aArray, uint32_t aSize) { MOZ_ASSERT(!mBitCount); uint32_t size = 0; for (uint32_t i = 0; i < aSize; i++) { size += Write(aArray[i]); } return size; }
Comment on attachment 8335749 [details] [diff] [review] 1_iso_media_compositor Review of attachment 8335749 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOCompositor.h @@ +109,5 @@ > + > + uint32_t GetTime(); > + > + // current fragment number > + uint32_t getCurFragmentNumber() { return mFragNum; } Capital function name "G"etCurFragmentNumber
Comment on attachment 8335749 [details] [diff] [review] 1_iso_media_compositor Review of attachment 8335749 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOCompositor.cpp @@ +39,5 @@ > + > +nsresult > +Fragmentation::GetFrame(uint32_t aIdx, EncodedFrame** aFrame) > +{ > + MOZ_ASSERT(aIdx < mFrames.Length()); You need run-time check aIdx as well if (aIdx >= mFrames.Length()) { return failed; } @@ +351,5 @@ > + nsresult rv; > + uint32_t size; > + uint64_t first_sample_offset = mOutputSize + mLastWrittenBoxPos; > + nsRefPtr<MovieFragmentBox> moof_box = new MovieFragmentBox(aTrackType, this); > + nsRefPtr<TrackBox> mdat_box = new TrackBox(aTrackType, this); moof_box and mdat_box are both local object in this function. I would suggest to use nsAutoPtr instread of nsRefPtr here. @@ +374,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (HasAudioTrack()) { > + mAudFrag->Flush(); > + } I am not sure why you need HasAudioTrack function. Logically, if mAudFrag is not nullptr, you have audio track; if mVideFrag is not nullptr, you have video track.(Correct me if I am wrong) why not just null check mAudFrag here if (mAudFrag) mAudFrag->Flush(); And remove HasXXXTrack functions
Comment on attachment 8335749 [details] [diff] [review] 1_iso_media_compositor Review of attachment 8335749 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOCompositor.h @@ +27,5 @@ > +public: > + Fragmentation(uint32_t aTrackType, uint32_t aFragDuration, > + TrackMetadataBase* aMetadata); > + > + nsresult GetFrame(uint32_t aIdx, EncodedFrame** aFrame); (Suggestion) Add a simple iterator for EncodedFrame access. @@ +35,5 @@ > + return NS_OK; > + } > + nsresult GetCSDFrame(EncodedFrame** aFrame); > + nsresult GetCSD(nsTArray<uint8_t>& aCSD); > + Do we really need these two function? @@ +37,5 @@ > + nsresult GetCSDFrame(EncodedFrame** aFrame); > + nsresult GetCSD(nsTArray<uint8_t>& aCSD); > + > + uint32_t GetCurrentAvailableSampleNumber(); > + uint32_t GetSampleNumberPerFragment(); GetSampleNumberPerFragment/ GetCurrentAvailableSampleNumber should be private. Suggest to make it an inline function @@ +136,5 @@ > +protected: > + uint32_t GetBufPos() { return mOutBuffer.Length(); } > + nsresult FlushBuf(); > + > +protected: There is no inherited. Let's make all data member private
Comment on attachment 8335749 [details] [diff] [review] 1_iso_media_compositor Review of attachment 8335749 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/TrackMetadataBase.h @@ +20,5 @@ > METADATA_VP8, > METADATA_OMXA, > METADATA_OMXV, > + METADATA_AVC, > + METADATA_AAC, Remove METADATA_OMXA,METADATA_OMXV,
Comment on attachment 8335751 [details] [diff] [review] 3_iso_media_writer Review of attachment 8335751 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOMediaWriter.cpp @@ +109,5 @@ > + return NS_OK; > + } > + if (mCompositor->HasVideoTrack() && !mVidFrag->EOS()) { > + return NS_OK; > + } Add ISOCompositor::IsEOS() public function, and move logic here into that function. Code here change to if (mCompositor->IsEOS()) mState = MUXING_DONE; @@ +135,5 @@ > + if (aMetadata->GetKind() == TrackMetadataBase::METADATA_AAC) { > + mCompositor->SetMetadata(aMetadata); > + mAudFrag = new Fragmentation(Audio_Track, > + mCompositor->GetFragmentDuration(), > + aMetadata); Try not to keep mAudFrag or mVidFrag pointer in ISOMediaWriter, not even call any function of Fragmentation in this class. Generate Fragmentation in ISOCompositor, use Fragmentation in ISOCompositor and box objects.
Comment on attachment 8335749 [details] [diff] [review] 1_iso_media_compositor Review of attachment 8335749 [details] [diff] [review]: ----------------------------------------------------------------- Please check my comment, export a member function as public only if you have a reason to do it. Next step, I am going to review box classes ::: content/media/fmp4/muxer/ISOCompositor.h @@ +32,5 @@ > + nsresult AddFrame(EncodedFrame* aFrame); > + nsresult SetEndOfStream() { > + mEOS = true; > + return NS_OK; > + } SetEndOfStream and EOS is not pair. Rename to SetEndOfStream with IsEndOfStream or SetEOS with IsEOS @@ +39,5 @@ > + > + uint32_t GetCurrentAvailableSampleNumber(); > + uint32_t GetSampleNumberPerFragment(); > + uint32_t GetType() { return mTrackType; } > + bool HasEnoughData(); Can we change this function as private? ContainerWriter just always call Flush. Inside Flush, we use HasEnoughData to determine whether flush data. @@ +115,5 @@ > + uint32_t GetFragmentDuration() { return mFragDuration; } > + nsresult SetFragmentDuration(uint32_t aDuration) { > + mFragDuration = aDuration; > + return NS_OK; > + } Why we need to have this public function? Can't we just setup duration of a fragmentation in ISOCompositor?
Comment on attachment 8335749 [details] [diff] [review] 1_iso_media_compositor Review of attachment 8335749 [details] [diff] [review]: ----------------------------------------------------------------- Please check my comment, export a member function as public only if you have a reason to do it. Next step, I am going to review box classes ::: content/media/fmp4/muxer/ISOCompositor.cpp @@ +48,5 @@ > +nsresult > +Fragmentation::GetCSD(nsTArray<uint8_t>& aCSD) > +{ > + if (!mCSDFrame) { > + return NS_ERROR_FAILURE; MOZ_ASSERT(mCSDFrame) I don't think the value of mCSDFrame is nullptr is a normal condition when you call GetCSD ::: content/media/fmp4/muxer/ISOCompositor.h @@ +32,5 @@ > + nsresult AddFrame(EncodedFrame* aFrame); > + nsresult SetEndOfStream() { > + mEOS = true; > + return NS_OK; > + } SetEndOfStream and EOS is not pair. Rename to SetEndOfStream with IsEndOfStream or SetEOS with IsEOS @@ +39,5 @@ > + > + uint32_t GetCurrentAvailableSampleNumber(); > + uint32_t GetSampleNumberPerFragment(); > + uint32_t GetType() { return mTrackType; } > + bool HasEnoughData(); Can we change this function as private? ContainerWriter just always call Flush. Inside Flush, we use HasEnoughData to determine whether flush data. @@ +115,5 @@ > + uint32_t GetFragmentDuration() { return mFragDuration; } > + nsresult SetFragmentDuration(uint32_t aDuration) { > + mFragDuration = aDuration; > + return NS_OK; > + } Why we need to have this public function? Can't we just setup duration of a fragmentation in ISOCompositor?
Comment on attachment 8335750 [details] [diff] [review] 2_iso_media_box Review of attachment 8335750 [details] [diff] [review]: ----------------------------------------------------------------- Hide all default constructor of Box concrete classes. ::: content/media/fmp4/muxer/ISOMediaBoxes.cpp @@ +71,5 @@ > + Fragmentation* frag = nullptr; > + if (mTrackType == Audio_Track) { > + frag = mCompositor->GetAudioFragment(); > + } else if (mTrackType == Video_Track) { > + frag = mCompositor->GetVideoFragment(); Can we have a ISOCompositor::GetFragement() function and move logic here into that function? ::: content/media/fmp4/muxer/ISOMediaBoxes.h @@ +71,5 @@ > +public: > + uint32_t size; > + nsCString boxType; > + > + Box(); Hide default constructor? @@ +75,5 @@ > + Box(); > + Box(const nsACString& aType, ISOCompositor* aCompositor); > + > + nsresult Write(); > + MOZ_OVERWRITE @@ +102,5 @@ > + FullBox(); > + FullBox(const nsACString& aType, uint8_t aVersion, uint32_t aFlags, > + ISOCompositor* aCompositor); > + > + nsresult Write(); MOZ_OVERWRITE @@ +103,5 @@ > + FullBox(const nsACString& aType, uint8_t aVersion, uint32_t aFlags, > + ISOCompositor* aCompositor); > + > + nsresult Write(); > + nsresult Find(const nsACString& aType, MuxerOperation** aOperation); MOZ_OVERWRITE @@ +162,5 @@ > +public: > + nsresult Generate(uint32_t* aBoxSize); > + nsresult Write(); > + uint32_t FirstSampleOffsetInTrackBox() { return mFirstSampleOffset; } > + Why not just use Box::size? I can see no reason to add a new data member, mFirstSampleOffset, here @@ +177,5 @@ > +#define flags_sample_duration_present 0x000100 > +#define flags_sample_size_present 0x000200 > +#define flags_sample_flags_present 0x000400 > +#define flags_sample_composition_time_offsets_present 0x000800 > + Where you use this definition? @@ +324,5 @@ > +protected: > + uint32_t mTrackType; > +}; > + > +class TimeToSampleBox : public FullBox { Should we move this class definition into .cpp file? Only declare classes that other compile units, ISOCompositor.cpp, need in header file. Please check all the classes in this header. You may need only declare these four classes in header FileTypeBox MovieBox MovieFragmentBox TrackBox
Comment on attachment 8335751 [details] [diff] [review] 3_iso_media_writer Review of attachment 8335751 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOMediaWriter.cpp @@ +135,5 @@ > + if (aMetadata->GetKind() == TrackMetadataBase::METADATA_AAC) { > + mCompositor->SetMetadata(aMetadata); > + mAudFrag = new Fragmentation(Audio_Track, > + mCompositor->GetFragmentDuration(), > + aMetadata); If you can move Fragmentation creating into ISOCompositor, please also remove ISOCompsitor::GetFragmentDuration().
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
(In reply to C.J. Ku[:CJKu] from comment #18) > Comment on attachment 8335749 [details] [diff] [review] > 1_iso_media_compositor > > Review of attachment 8335749 [details] [diff] [review]: > > I am not sure why you need HasAudioTrack function. > Logically, if mAudFrag is not nullptr, you have audio track; if mVideFrag is > not nullptr, you have video track.(Correct me if I am wrong) > > why not just null check mAudFrag here > if (mAudFrag) > mAudFrag->Flush(); > > And remove HasXXXTrack functions Meta data should be the only way to know if this is audio or video only before Randy's video path complete. Once Randy's part complete, I will change to se Randy's new interface. Others fixed.
Target Milestone: 1.3 Sprint 6 - 12/6 → 1.3 Sprint 5 - 11/22
(In reply to C.J. Ku[:CJKu] from comment #19) > Comment on attachment 8335749 [details] [diff] [review] > > + return NS_OK; > > + } > > + nsresult GetCSDFrame(EncodedFrame** aFrame); > > + nsresult GetCSD(nsTArray<uint8_t>& aCSD); > > + > > Do we really need these two function? GetCSDFrame, removed. GetCSD, yes, it needs to get codec specific data. > > @@ +37,5 @@ > > + nsresult GetCSDFrame(EncodedFrame** aFrame); > > + nsresult GetCSD(nsTArray<uint8_t>& aCSD); > > + > > + uint32_t GetCurrentAvailableSampleNumber(); > > + uint32_t GetSampleNumberPerFragment(); > > GetSampleNumberPerFragment/ GetCurrentAvailableSampleNumber should be > private. Suggest to make it an inline function GetSampleNumberPerFragment is ok. GetCurrentAvailableSampleNumber will be used other class.
(In reply to C.J. Ku[:CJKu] from comment #20) > Comment on attachment 8335749 [details] [diff] [review] > 1_iso_media_compositor > > Review of attachment 8335749 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/encoder/TrackMetadataBase.h > @@ +20,5 @@ > > METADATA_VP8, > > METADATA_OMXA, > > METADATA_OMXV, > > + METADATA_AVC, > > + METADATA_AAC, > > Remove METADATA_OMXA,METADATA_OMXV, They are on Shelly's part, they will be gone after merge to Shelly's new version.
(In reply to Alfredo Yang from comment #27) > > GetSampleNumberPerFragment/ GetCurrentAvailableSampleNumber should be > > private. Suggest to make it an inline function > > GetSampleNumberPerFragment is ok. > GetCurrentAvailableSampleNumber will be used other class. I would prefer create a iterator to iterate frames in Fragmentation, and remove GetCurrentAvailableSampleNumber/ GetFrame functions
Comment on attachment 8335750 [details] [diff] [review] 2_iso_media_box Review of attachment 8335750 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOMediaBoxes.h @@ +36,5 @@ > + * |---> MovieHeaderBox (full box) > + * |---> TrakBox (container) > + * |---> MovieExtendsBox (container) > + */ > +class MuxerOperation { Move the definition of this class to another header, e.g. MuxerOperation.h Include MuxerOperation.h in MP4ESDS.h, instead of ISOMediaBoxes.h
Comment on attachment 8335750 [details] [diff] [review] 2_iso_media_box Review of attachment 8335750 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOMediaBoxes.cpp @@ +31,5 @@ > + ~BoxSizeChecker() { > + uint32_t cur_size = mCompositor->GetBufPos(); > + if ((cur_size - ori_size) != box_size) { > + MOZ_ASSERT(false); > + } MOZ_ASSERT((cur_size - ori_size) == box_size); @@ +32,5 @@ > + uint32_t cur_size = mCompositor->GetBufPos(); > + if ((cur_size - ori_size) != box_size) { > + MOZ_ASSERT(false); > + } > + mCompositor->mLastWrittenBoxPos = mCompositor->mOutBuffer.Length(); Without this line, you may name this class as BoxSizeChecker. With this line, BoxSizeTracer is better naming ::: content/media/fmp4/muxer/ISOMediaBoxes.h @@ +49,5 @@ > + > + // Write data to stream. > + virtual nsresult Write() > + { > + return NS_ERROR_FAILURE; NS_OK or NS_ERROR_FAILURE?
Comment on attachment 8335750 [details] [diff] [review] 2_iso_media_box Review of attachment 8335750 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOMediaBoxes.h @@ +58,5 @@ > + // It can only look downstream including itself and the box in the boxes > + // list if exists. It can't look upstream. > + virtual nsresult Find(const nsACString& aType, MuxerOperation** aOperation) > + { > + return NS_ERROR_FAILURE; NS_OK or NS_ERROR_FAILURE? I think it should be NS_OK even if you can't find an operation in this box. Caller should null check aOperation. @@ +275,5 @@ > + TrackExtendsBox(uint32_t aType, ISOCompositor* aCompositor); > + > +protected: > + uint32_t mTrackType; > + MetaHelper mMeta; Define a helper object as data member is weird. Use it as local object in function.
Comment on attachment 8335750 [details] [diff] [review] 2_iso_media_box Review of attachment 8335750 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/muxer/ISOMediaBoxes.cpp @@ +116,5 @@ > + > +uint32_t > +TrackRunBox::fillSampleTable() > +{ > + uint32_t table_size = 0; Move local variable definition to #127 @@ +122,5 @@ > + if (mTrackType == Audio_Track) { > + frag = mCompositor->GetAudioFragment(); > + } else { > + frag = mCompositor->GetVideoFragment(); > + } frag = mCompositor->GetFragement(); ASSERT(sample_info_table == nullptr); @@ +126,5 @@ > + } > + sample_info_table = new tbl[frag->GetCurrentAvailableSampleNumber()]; > + for (uint32_t i = 0; i < frag->GetCurrentAvailableSampleNumber(); i++) { > + nsRefPtr<EncodedFrame> frame; > + frag->GetFrame(i, getter_AddRefs(frame)); memcpy(sample_info_table[i], 0, sizeof(tbl)); And remove all 0 assignment code below @@ +144,5 @@ > + if (mTrackType == Audio_Track) { > + frag = mCompositor->GetAudioFragment(); > + } else { > + frag = mCompositor->GetVideoFragment(); > + } frag = mCompositor->GetFragement(); ::: content/media/fmp4/muxer/ISOMediaBoxes.h @@ +45,5 @@ > + virtual nsresult Generate(uint32_t* aBoxSize) > + { > + return NS_ERROR_FAILURE; > + } > + In your design, Generate function of each object should be called once. Have a debug code here to make sure no one disobey this rull virtual nsresult Generate(uint32_t* aBoxSize) { MOZ_ASSERT(mGenerated); mGenerated = true; return NS_OK; }
Attached patch 1_iso_media_compositor (obsolete) — — Splinter Review
Attachment #8335749 - Attachment is obsolete: true
Attachment #8335750 - Attachment is obsolete: true
Attachment #8335751 - Attachment is obsolete: true
Attached patch 2_iso_media_box (obsolete) — — Splinter Review
Attached patch 3_iso_media_writer (obsolete) — — Splinter Review
Attached patch 1_iso_media_muxer (obsolete) — — Splinter Review
Attachment #8337608 - Attachment is obsolete: true
Attachment #8337609 - Attachment is obsolete: true
Attachment #8337610 - Attachment is obsolete: true
Attached patch 2_iso_media_writer (obsolete) — — Splinter Review
ISOMediaWriter 1. aggregates one ISOCompositor. 2. aggregates one-and-only-on Fragmentation 3. A container writer to receive encoded frames from Track encoder. ISOCompositor 1. associate one Fragmentation, which is hosted by ISOMediaWriter. 2. Create box objects to serialize muxed data. Fragmentation 1. is a buffer object to hold encoded audio frames 1. is a buffer object to hold encoded video frames
Per discussion offline - this is being moved out of scope for 1.3. We will target this feature for 1.4. However, we don't block on targeted features, so this is no longer a blocker.
blocking-b2g: 1.3+ → -
Move target milestone to future for the release in 1.4
Target Milestone: 1.3 Sprint 5 - 11/22 → Future
meant to rest target milestone to "---" and plan the sprint in v1.4
blocking-b2g: - → ---
Target Milestone: Future → ---
Attached patch fmp4_1_iso_media_box (obsolete) — — Splinter Review
Attachment #8337613 - Attachment is obsolete: true
Attachment #8337614 - Attachment is obsolete: true
Attached patch fmp4_2_iso_media_writer (obsolete) — — Splinter Review
Attached patch fmp4_1_iso_media_box (obsolete) — — Splinter Review
Attachment #8340202 - Attachment is obsolete: true
Comment on attachment 8340203 [details] [diff] [review] fmp4_2_iso_media_writer Review of attachment 8340203 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/fmp4_muxer/ISOMediaWriter.cpp @@ +9,5 @@ > + > +#undef LOG > +#ifdef MOZ_WIDGET_GONK > +#include <android/log.h> > +#define LOG(args...) __android_log_print(ANDROID_LOG_INFO, "MediaEncoder", ## args); Change log tag to "ISOMediaWriter"? ::: content/media/encoder/fmp4_muxer/ISOMediaWriter.h @@ +40,5 @@ > + * > + * Following is the details of each state. > + * MUXING_HEAD: > + * It collects the metadata to generate a moov. The state transits to > + * MUXING_HEAD after output moov blob. Do you mean transits to 'MUXING_FRAG' here? @@ +44,5 @@ > + * MUXING_HEAD after output moov blob. > + * > + * MUXING_FRAG: > + * It collects enough audio/video data to generate a fragement blob. This > + * will be repeated until END_OF_STREAM and then transiting to MUXING_DONE. Sound odd to me... Doesn't it transit to 'MUXING_MFRA' before 'MUXING_DONE'? ::: content/media/encoder/fmp4_muxer/moz.build @@ +8,5 @@ > + 'ISOMediaWriter.h', > + 'ISOTrackMetadata.h', > +] > + > +SOURCES += [ UNIFIED_SOURCES?
Whiteboard: [ft:media-recording] → [ft:multimedia-platform]
Comment on attachment 8340203 [details] [diff] [review] fmp4_2_iso_media_writer Review of attachment 8340203 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/fmp4_muxer/ISOMediaWriter.cpp @@ +16,5 @@ > +#endif > + > +namespace mozilla { > + > +const static uint32_t FRAG_DURATION = 10; 10 sec? maybe FRAGMENT_DURATION_S @@ +135,5 @@ > +ISOMediaWriter::GetContainerData(nsTArray<nsTArray<uint8_t> >* aOutputBufs, > + uint32_t aFlags) > +{ > + if (mBlobReady) { > + mBlobReady = false; Let the mBlobReady to be the mCompositor attribute? ::: content/media/encoder/moz.build @@ +3,5 @@ > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +PARALLEL_DIRS += ['fmp4_muxer'] Build flag?
Comment on attachment 8340209 [details] [diff] [review] fmp4_1_iso_media_box Review of attachment 8340209 [details] [diff] [review]: ----------------------------------------------------------------- Can we separate each class into several patches?
Attached patch 1_fmp4_muxeroperation (obsolete) — — Splinter Review
Attachment #8340203 - Attachment is obsolete: true
Attachment #8340209 - Attachment is obsolete: true
Attachment #8347727 - Flags: review?(cpearce)
Attached patch 2_fmp4_iso_boxes (obsolete) — — Splinter Review
Attachment #8347728 - Flags: review?(cpearce)
Attached patch 3_fmp4_avc_box (obsolete) — — Splinter Review
Attachment #8347729 - Flags: review?(cpearce)
Attached patch 4_fmp4_mp4_esds_box (obsolete) — — Splinter Review
Attachment #8347730 - Flags: review?(cpearce)
Attached patch 5_fmp4_iso_control (obsolete) — — Splinter Review
Attachment #8347731 - Flags: review?(cpearce)
Attached patch 6_fmp4_iso_media_writer (obsolete) — — Splinter Review
Attachment #8347732 - Flags: review?(cpearce)
Comment on attachment 8347727 [details] [diff] [review] 1_fmp4_muxeroperation Review of attachment 8347727 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Only style/grammar fixes. Assume r=cpearce once the following are fixed. ::: content/media/encoder/fmp4_muxer/MuxerOperation.h @@ +11,5 @@ > + > +namespace mozilla { > + > +/** > + * The interface for ISO box. All Boxes inherited this interface. s/All Boxes inherited this interface./All Boxes inherit from this interface./ @@ +12,5 @@ > +namespace mozilla { > + > +/** > + * The interface for ISO box. All Boxes inherited this interface. > + * Generate() and Write() are needed to be called to producet a complete box. s/producet/produce/ @@ +14,5 @@ > +/** > + * The interface for ISO box. All Boxes inherited this interface. > + * Generate() and Write() are needed to be called to producet a complete box. > + * > + * Generate() will generate all the data structure and its size. s/Generate() will generate all the data structure and its size./Generate() will generate all the data structures and their size./ @@ +38,5 @@ > + > + // Generate data of this box and its contained box, and calculate box size. > + virtual nsresult Generate(uint32_t* aBoxSize) > + { > + MOZ_ASSERT(0); Could you have this as a pure virtual function, e.g.: virtual nsresult Generate(uint32_t* aBoxSize) = 0; Then the compiler will refuse to compile code that instantiates sub-classes of this class that don't implement these methods. This means you'll fail at compile time, rather than failing at runtime (in debug builds only). The same thing can be done to Write() and Find() below. @@ +51,5 @@ > + } > + > + // Find the box type via its name (name is the box type defined in 14496-12; > + // for example, 'moov' is the name of MovieBox). > + // It can only look downstream including itself and the box in the boxes Instead of using the words "downstream" and "upstream", I recommend you use the terms "in child boxes" and "in parent boxes", as this is the language most commonly used in graph/tree data structures. @@ +54,5 @@ > + // for example, 'moov' is the name of MovieBox). > + // It can only look downstream including itself and the box in the boxes > + // list if exists. It can't look upstream. > + virtual nsresult Find(const nsACString& aType, > + nsTArray<nsRefPtr<MuxerOperation> >& aOperations) You don't actually need the space between the two '>' characters, e.g.: nsTArray<nsRefPtr<MuxerOperation>>& is fine. See "Foo<Bar>>" in the following page: https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code#C.2B.2B_language_features
Attachment #8347727 - Flags: review?(cpearce) → review+
It's also worth pointing out that the mp4_demuxer that we have in the tree will probably not yet demux files muxed with this muxer as it enforces the constraints imposed by Media Source Extensions [https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/isobmff-byte-stream-format.html], whereas I assume this muxer should not mux to those constraints.
A general comment; all classes *must* have a MOZ_COUNT_CTOR($className) added to their constructor, and MOZ_COUNT_DTOR($className) added to their destructor. For example, see: http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/WMFByteStream.cpp#120 This enables you to detect memory leaks and reference cycles by running (on desktop at least) with XPCOM_MEM_LEAK_LOG=2 environment variable. If there's a leak, logging on the console will list what leaks when Firefox shuts down.
Comment on attachment 8347728 [details] [diff] [review] 2_fmp4_iso_boxes Review of attachment 8347728 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good. Mostly style "nits". r=cpearce with these addressed. ::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.cpp @@ +14,5 @@ > + > +namespace mozilla { > + > +// 14496-12 6.2.2 'Data Types and fields' > +const uint32_t iso_matrix[] = { 0x00010000, 0, 0, 0, I think you should lay this out with three array entries per line, so that it's clear that this is the identity matrix. @@ +181,5 @@ > +} > + > +TrackRunBox::~TrackRunBox() > +{ > + if (sample_info_table) { If you make sample_info_table an nsAutoArrayPtr<tbl> then you don't need this explicit delete. Also people don't have to go looking for an explicit delete to be sure that you remembered to delete the memory. In general, if you're using operator 'new' you should be storing the allocated memory in an nsAutoArrayPtr, nsAutoPtr, nsRefPtr, nsCOMPtr, or mozilla::RefPtr, rather than using an explicit delete. @@ +207,5 @@ > + size += sizeof(base_data_offset); > + } > + if (flags.to_ulong() | default_sample_duration_present) { > + if (mTrackType == Video_Track) { > + default_sample_duration = mMeta.mVidMeta->VideoFrequence / mMeta.mVidMeta->FrameRate; "Frequence" is not an English word. Do you mean VideoFrequence to be the sum of the number of video frames? That would be frame count. Whereas "frequency" refers to Hz (i.e. samples per second), which is different to a count. ::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.h @@ +43,5 @@ > + // Check if this box is the one we find. > + nsresult Find(const nsACString& aType, > + nsTArray<nsRefPtr<MuxerOperation> >& aOperations) MOZ_OVERRIDE; > + > + // A helper class to check box wirten bytes number; it will compare s/wirten/written/ @@ +111,5 @@ > + > +protected: > + DefaultContainerImpl(const nsACString& aType, ISOControl* aControl); > + > +protected: You don't need a second "protected:" here, you have on on line 112 above. @@ +123,5 @@ > + nsCString major_brand; // four chars > + uint32_t minor_version; > + nsTArray<nsCString> compatible_brands; > + > +public: You don't need a second "public:" here. @@ +156,5 @@ > +#define flags_sample_flags_present 0x000400 > +#define flags_sample_composition_time_offsets_present 0x000800 > + > +// flag for TrackRunBox::tbl::sample_flags and TrackExtendsBox::default_sample_flags > +// which is defined in 14496-12 8.8.3.1. (this define is from GPAC) I could not find this macro/define in the GPAC code base. You should not copy the code directly from GPAC, as its open source license is not the same as ours; we can only dynamically link LGPL code. You need to re-implement this macro, rather than copying it from GPAC. @@ +189,5 @@ > + > +protected: > + uint32_t fillSampleTable(); > + > +protected: Don't need second "protected:" here. @@ +242,5 @@ > +class MovieFragmentHeaderBox : public FullBox { > +public: > + uint32_t sequence_number; > + > +public: Don't need the second "public:" @@ +275,5 @@ > + uint32_t default_sample_duration; > + uint32_t default_sample_size; > + uint32_t default_sample_flags; > + > +public: Don't need the second "public:" @@ +305,5 @@ > + } tbl; > + > + uint32_t entry_count; > + tbl* sample_tbl; > +public: Don't need the second "public:". There's more in the classes below. If you're using extra "public","protected" declarations to partition ISO-BMFF specified data from the functions required to implement a parent class, or from functions needed by this class, then I recommend you make it obvious by adding a comment saying what interface you're implementing. That is, rather than rely on visibility declarations to separate things into logical chunks, use comments. For example this class: http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/WMFByteStream.h I have comments like: // IMFByteStream Methods. To denote that the following methods implement the IMFByteStream interface. @@ +361,5 @@ > + uint32_t mTrackType; > +}; > + > +// 14496-12 8.5.2 'Sample Description Box' > +// This is the basd calss for VisualSampleEntry and MP4AudioSampleEntry. s/basd calss/base class/ @@ +420,5 @@ > +// SampleSizeBox and > +// ChunkOffsetBox. > +class SampleTableBox : public DefaultContainerImpl { > +public: > + SampleTableBox(uint32_t aType, ISOControl* aControl); You don't need Write() and Generate() functions on this box? @@ +446,5 @@ > +// Box type: 'dref' > +class DataReferenceBox : public FullBox { > +public: > + uint32_t entry_count; > + nsTArray<nsAutoPtr<DataEntryUrlBox> > urls; s/> >/>>/ There are lots of these throughout the code.
Attachment #8347728 - Flags: review?(cpearce) → review+
Comment on attachment 8347729 [details] [diff] [review] 3_fmp4_avc_box Review of attachment 8347729 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/fmp4_muxer/AVCBox.h @@ +5,5 @@ > + > +#ifndef AVCBox_h_ > +#define AVCBox_h_ > + > +#include <bitset> You don't use <bitset>, so I don't think you need to include it here. @@ +28,5 @@ > + // These data are generated by encoder and we encapusulated the generated > + // bitstream into box directly. > + nsTArray<uint8_t> avcConfig; > + > +public: You don't need the extra "public" here, and below.
Attachment #8347729 - Flags: review?(cpearce) → review+
Attachment #8347730 - Flags: review?(cpearce) → review+
Comment on attachment 8347731 [details] [diff] [review] 5_fmp4_iso_control Review of attachment 8347731 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r=cpearce with these nits addressed. ::: content/media/encoder/fmp4_muxer/ISOControl.cpp @@ +65,5 @@ > + mFragArray.AppendElement(); > + } > + mFragArray.LastElement().AppendElement(aFrame); > + > + /* Don't include commented out code in your patch. Fix the code and uncomment it, or move it to another patch. @@ +198,5 @@ > +{ > + for (uint32_t i = 0; i < mMetaArray.Length() ; i++) { > + if (mMetaArray[i]->GetKind() == TrackMetadataBase::METADATA_AVC) { > + NS_ENSURE_ARG_POINTER(aVidMeta); > + NS_IF_ADDREF(*aVidMeta = static_cast<AVCTrackMetadata*>(mMetaArray[i].get())); Where do you Release() this? I think you should make the signature for this function: nsresult ISOControl::GetVideoMetadata(nsRefPtr<AVCTrackMetadata>& aVidMeta) So that you enforce that the caller properly handles refcounting aVidMeta. Do the same for GetAudioMetadata() please. ::: content/media/encoder/fmp4_muxer/ISOControl.h @@ +27,5 @@ > +class FragmentBuffer { > +public: > + // aTrackType: it could be Audio_Track or Video_Track. > + // aFragDuration: it is the fragmentation duration, it needs to be the same > + // value for both. Document what units aFragDuration is in. Samples? Seconds? Microseconds? @@ +31,5 @@ > + // value for both. > + FragmentBuffer(uint32_t aTrackType, uint32_t aFragDuration, > + TrackMetadataBase* aMetadata); > + > + // Get samples of first fragmentation The comment should say that this causes the FragmentBuffer to drop all references to the EncodedFrames, so the caller is now responsible for their destruction. @@ +40,5 @@ > + // number is enough, it will append a new fragment element. And the new > + // sample will be added to the new fragment element of mFragArray. > + nsresult AddFrame(EncodedFrame* aFrame); > + > + // Get total sample size of first complete fragmentation size. "fragmentation" in English means "the process or state of breaking or being broken into fragments." You're not using the word correctly here. You should use the word "fragments" to describe the things that are fragmented, not "fragmentation". There are many instances of this mistake throughout this patch. @@ +59,5 @@ > + return NS_OK; > + } > + bool EOS() { return mEOS; } > + > + // CSD (codec specific data), it is generated by encoder and the data is s/the data is depedes/the data depends/ @@ +74,5 @@ > + > +private: > + uint32_t mTrackType; > + > + // fragmentation duration, it counts by seconds. Isn't it a bad idea to count the duration by seconds? The duration is unlikely to be exactly an even multiple of 1 second. Surely you should be using microseconds, or a rational number/fraction? @@ +78,5 @@ > + // fragmentation duration, it counts by seconds. > + uint32_t mFragDuration; > + > + // Current time. > + // Audio time is based on sample rate. Is the audio time the sum of encoded samples? If so say so, to be clearer. @@ +84,5 @@ > + uint64_t mMediaStartTime; > + > + // Current fragmentation number. It will be increase when a new element of > + // mFragArray is created. (Note: it only means the fragment number of current > + // accmultated frames, not the current 'creating' fragment.) accumulated @@ +106,5 @@ > + > +class Box; > + > +/** > + * ISOCOmpositor will be carried to each box when box is created. It is the main Did you mean "ISOControl will be passed to each box when box is created." ? @@ +132,5 @@ > + // for random seeking; for example, Google Chrome. Others could recognize the > + // video without it as streaming case (for example, Ubuntu Video). According > + // to spec 14496-12 C.8 'Construction of fragmented movies', it is optional > + // but it may be a good idea to implement it. > + nsresult GenerateMfra() { return NS_OK; } You could return NS_ERROR_NOT_IMPLEMENTED? Are you planning on implementing this? It would be a good idea if our <video> players can't seek if this box isn't included. @@ +167,5 @@ > + > + // This is called by GetContainerData and swap the buffer to aOutBuf. > + nsresult GetBuf(nsTArray<uint8_t>& aOutBuf); > + > + // This function is from Android stagefright. If you copy any code, you must conform to the license of that code. That means reproducing the copyright notice of the code. Easier to just re-implement the function with identical output for small functions like this I think. @@ +202,5 @@ > + nsresult FlushBuf(); > + > +private: > + FragmentBuffer* mAudioFragmentBuffer; > + FragmentBuffer* mVideoFragmentBuffer; You should comment that these objects are owned by the containerwriter, so that you don't need to worry about when they're freed, or about the pointers going stale.
Attachment #8347731 - Flags: review?(cpearce) → review+
Comment on attachment 8347732 [details] [diff] [review] 6_fmp4_iso_media_writer Review of attachment 8347732 [details] [diff] [review]: ----------------------------------------------------------------- Nice work. :) ::: content/media/encoder/fmp4_muxer/ISOMediaWriter.cpp @@ +21,5 @@ > +namespace mozilla { > + > +const static uint32_t FRAG_DURATION = 2; > + > +class ISOMediaWriterRunnable : public nsRunnable { You don't use this anywhere. Delete it, and add it back in once you need it. And the reference to mStateMachineRunner below. @@ +37,5 @@ > + return NS_OK; > + } > +private: > + // Don't hold refernce here to avoid cycle reference. > + ISOMediaWriter* mWriter; This comment is misleading. ISOMediaWriter isn't refcounted, so you can't hold a strong reference, so you can't cause a cycle. So if or when you add this code back in, you should fix this comment. @@ +81,5 @@ > + } > + case MUXING_MFRA: > + { > + rv = mControl->GenerateMfra(); > + NS_ENSURE_SUCCESS(rv, rv); If you change GenerateMfra to return NS_ERROR_NOT_IMPLEMENTED you'll need to adjust this here. @@ +128,5 @@ > + } > + > + // Encoder should send CSD (codec specific data) frame before sending the > + // audio/video frames. When CSD data is ready, it is sufficient to generate a > + // moov data. If encoder doesn't send CSD yet, mxuer needs to wait before s/mxuer/muxer/ @@ +177,5 @@ > +bool > +ISOMediaWriter::ReadyToRunState() > +{ > + bool bReadyToMux = true; > + if (mType & (Audio_Track | Video_Track)) { If you're trying to check that mType contains both Audio_Track and Video_Track bits set, this won't do it. This will return non-zero if either or both Audio_Track and Video_Track are set. That is, ((mType & (Audio_Track | Video_Track)) returns non-zero when either Audio_Track and Video_Track, or both are set, not only when both are set. If you want to check for both together, you need something like ((mType & (Audio_Track | Video_Track)) == (Audio_Track | Video_Track)) or ((mType & Audio_Track) && (mType & Video_Track)) ::: content/media/encoder/fmp4_muxer/ISOMediaWriter.h @@ +72,5 @@ > + // 2. Get EOS signal. > + bool ReadyToRunState(); > + > + nsAutoPtr<ISOControl> mControl; > + nsAutoPtr<FragmentBuffer> mAudioFragmentBuffer; You should document somewhere where these get deleted (in MediaDataBox::Write()) it took me a while to figure that out. ::: content/media/encoder/fmp4_muxer/moz.build @@ +8,5 @@ > + 'ISOMediaWriter.h', > + 'ISOTrackMetadata.h', > +] > + > +SOURCES += [ Make these UNIFIED_SOURCES. You may need to change some other files in order to build with this as UNIFIED_SOURCES, but it will make the build faster overall.
Attachment #8347732 - Flags: review?(cpearce) → review+
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Blocks: 879668
(In reply to Chris Pearce (:cpearce) from comment #55) > Comment on attachment 8347727 [details] [diff] [review] > 1_fmp4_muxeroperation > > Review of attachment 8347727 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good! Only style/grammar fixes. > > Assume r=cpearce once the following are fixed. > > ::: content/media/encoder/fmp4_muxer/MuxerOperation.h > @@ +11,5 @@ > > + > > +namespace mozilla { > > + > > +/** > > + * The interface for ISO box. All Boxes inherited this interface. > > s/All Boxes inherited this interface./All Boxes inherit from this interface./ > Thanks for review. It's fixed. > @@ +12,5 @@ > > +namespace mozilla { > > + > > +/** > > + * The interface for ISO box. All Boxes inherited this interface. > > + * Generate() and Write() are needed to be called to producet a complete box. > > s/producet/produce/ > Fixed. > @@ +14,5 @@ > > +/** > > + * The interface for ISO box. All Boxes inherited this interface. > > + * Generate() and Write() are needed to be called to producet a complete box. > > + * > > + * Generate() will generate all the data structure and its size. > > s/Generate() will generate all the data structure and its size./Generate() > will generate all the data structures and their size./ > Fixed. > @@ +38,5 @@ > > + > > + // Generate data of this box and its contained box, and calculate box size. > > + virtual nsresult Generate(uint32_t* aBoxSize) > > + { > > + MOZ_ASSERT(0); > > Could you have this as a pure virtual function, e.g.: > > virtual nsresult Generate(uint32_t* aBoxSize) = 0; > > Then the compiler will refuse to compile code that instantiates sub-classes > of this class that don't implement these methods. This means you'll fail at > compile time, rather than failing at runtime (in debug builds only). > > The same thing can be done to Write() and Find() below. > Fixed. > @@ +51,5 @@ > > + } > > + > > + // Find the box type via its name (name is the box type defined in 14496-12; > > + // for example, 'moov' is the name of MovieBox). > > + // It can only look downstream including itself and the box in the boxes > > Instead of using the words "downstream" and "upstream", I recommend you use > the terms "in child boxes" and "in parent boxes", as this is the language > most commonly used in graph/tree data structures. > Fixed. > @@ +54,5 @@ > > + // for example, 'moov' is the name of MovieBox). > > + // It can only look downstream including itself and the box in the boxes > > + // list if exists. It can't look upstream. > > + virtual nsresult Find(const nsACString& aType, > > + nsTArray<nsRefPtr<MuxerOperation> >& aOperations) > > You don't actually need the space between the two '>' characters, e.g.: > nsTArray<nsRefPtr<MuxerOperation>>& is fine. > > See "Foo<Bar>>" in the following page: > https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code#C.2B. > 2B_language_features Fixed.
Attached patch 1_fmp4_muxeroperation (obsolete) — — Splinter Review
Attachment #8347727 - Attachment is obsolete: true
Attachment #8347728 - Attachment is obsolete: true
Attachment #8347729 - Attachment is obsolete: true
Attachment #8347730 - Attachment is obsolete: true
Attachment #8347731 - Attachment is obsolete: true
Attachment #8347732 - Attachment is obsolete: true
(In reply to Chris Pearce (:cpearce) from comment #56) > [https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/isobmff-byte- > stream-format.html], whereas I assume this muxer should not mux to those > constraints. Thanks for point it out. It appears that some extra boxes (like tfdt) and random access point need to be added. I'll file a following bug for that.
(In reply to Chris Pearce (:cpearce) from comment #57) > A general comment; all classes *must* have a MOZ_COUNT_CTOR($className) > added to their constructor, and MOZ_COUNT_DTOR($className) added to their > destructor. For example, see: > http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/ > WMFByteStream.cpp#120 > > This enables you to detect memory leaks and reference cycles by running (on > desktop at least) with XPCOM_MEM_LEAK_LOG=2 environment variable. If there's > a leak, logging on the console will list what leaks when Firefox shuts down. Fixed, except for some base classes for inherited only.
(In reply to Chris Pearce (:cpearce) from comment #58) > Comment on attachment 8347728 [details] [diff] [review] > 2_fmp4_iso_boxes > > Review of attachment 8347728 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall looks good. Mostly style "nits". > > r=cpearce with these addressed. > > ::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.cpp > @@ +14,5 @@ > > + > > +namespace mozilla { > > + > > +// 14496-12 6.2.2 'Data Types and fields' > > +const uint32_t iso_matrix[] = { 0x00010000, 0, 0, 0, > > I think you should lay this out with three array entries per line, so that > it's clear that this is the identity matrix. > Fixed. > @@ +181,5 @@ > > +} > > + > > +TrackRunBox::~TrackRunBox() > > +{ > > + if (sample_info_table) { > > If you make sample_info_table an nsAutoArrayPtr<tbl> then you don't need > this explicit delete. Also people don't have to go looking for an explicit > delete to be sure that you remembered to delete the memory. > > In general, if you're using operator 'new' you should be storing the > allocated memory in an nsAutoArrayPtr, nsAutoPtr, nsRefPtr, nsCOMPtr, or > mozilla::RefPtr, rather than using an explicit delete. > Fixed. > @@ +207,5 @@ > > + size += sizeof(base_data_offset); > > + } > > + if (flags.to_ulong() | default_sample_duration_present) { > > + if (mTrackType == Video_Track) { > > + default_sample_duration = mMeta.mVidMeta->VideoFrequence / mMeta.mVidMeta->FrameRate; > > "Frequence" is not an English word. Do you mean VideoFrequence to be the sum > of the number of video frames? That would be frame count. Whereas > "frequency" refers to Hz (i.e. samples per second), which is different to a > count. > Fixed. I installed a spell check tool on vim, it should be able to reduce such kind of problems. > ::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.h > @@ +43,5 @@ > > + // Check if this box is the one we find. > > + nsresult Find(const nsACString& aType, > > + nsTArray<nsRefPtr<MuxerOperation> >& aOperations) MOZ_OVERRIDE; > > + > > + // A helper class to check box wirten bytes number; it will compare > > s/wirten/written/ > Fixed. > @@ +156,5 @@ > > +#define flags_sample_flags_present 0x000400 > > +#define flags_sample_composition_time_offsets_present 0x000800 > > + > > +// flag for TrackRunBox::tbl::sample_flags and TrackExtendsBox::default_sample_flags > > +// which is defined in 14496-12 8.8.3.1. (this define is from GPAC) > > I could not find this macro/define in the GPAC code base. > Because I changed its name. I rewritten this function to avoid license problem. > @@ +305,5 @@ > > + } tbl; > > + > > + uint32_t entry_count; > > + tbl* sample_tbl; > > +public: > > Don't need the second "public:". There's more in the classes below. > > If you're using extra "public","protected" declarations to partition > ISO-BMFF specified data from the functions required to implement a parent > class, or from functions needed by this class, then I recommend you make it > obvious by adding a comment saying what interface you're implementing. That > is, rather than rely on visibility declarations to separate things into > logical chunks, use comments. > > For example this class: > http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/ > WMFByteStream.h > > I have comments like: > // IMFByteStream Methods. > > To denote that the following methods implement the IMFByteStream interface. > Fixed. > @@ +361,5 @@ > > + uint32_t mTrackType; > > +}; > > + > > +// 14496-12 8.5.2 'Sample Description Box' > > +// This is the basd calss for VisualSampleEntry and MP4AudioSampleEntry. > > s/basd calss/base class/ > Fixed. > @@ +420,5 @@ > > +// SampleSizeBox and > > +// ChunkOffsetBox. > > +class SampleTableBox : public DefaultContainerImpl { > > +public: > > + SampleTableBox(uint32_t aType, ISOControl* aControl); > > You don't need Write() and Generate() functions on this box? > No, SampleTableBox is a container box and all of its operations are same as its base class DefaultContainerImpl. I write this class SampleTableBox is for easy to understand/debug the box structure on codes. > @@ +446,5 @@ > > +// Box type: 'dref' > > +class DataReferenceBox : public FullBox { > > +public: > > + uint32_t entry_count; > > + nsTArray<nsAutoPtr<DataEntryUrlBox> > urls; > > s/> >/>>/ > > There are lots of these throughout the code. Fixed.
Attached patch 2_fmp4_iso_boxes (obsolete) — — Splinter Review
(In reply to Chris Pearce (:cpearce) from comment #59) > Comment on attachment 8347729 [details] [diff] [review] > 3_fmp4_avc_box > > Review of attachment 8347729 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/encoder/fmp4_muxer/AVCBox.h > @@ +5,5 @@ > > + > > +#ifndef AVCBox_h_ > > +#define AVCBox_h_ > > + > > +#include <bitset> > > You don't use <bitset>, so I don't think you need to include it here. > Fixed. > @@ +28,5 @@ > > + // These data are generated by encoder and we encapusulated the generated > > + // bitstream into box directly. > > + nsTArray<uint8_t> avcConfig; > > + > > +public: > > You don't need the extra "public" here, and below. Fixed.
Attached patch 3_fmp4_avc_box (obsolete) — — Splinter Review
(In reply to Chris Pearce (:cpearce) from comment #60) > Comment on attachment 8347731 [details] [diff] [review] > 5_fmp4_iso_control > > Review of attachment 8347731 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. r=cpearce with these nits addressed. > > ::: content/media/encoder/fmp4_muxer/ISOControl.cpp > @@ +65,5 @@ > > + mFragArray.AppendElement(); > > + } > > + mFragArray.LastElement().AppendElement(aFrame); > > + > > + /* > > Don't include commented out code in your patch. Fix the code and uncomment > it, or move it to another patch. > Fixed. > @@ +198,5 @@ > > +{ > > + for (uint32_t i = 0; i < mMetaArray.Length() ; i++) { > > + if (mMetaArray[i]->GetKind() == TrackMetadataBase::METADATA_AVC) { > > + NS_ENSURE_ARG_POINTER(aVidMeta); > > + NS_IF_ADDREF(*aVidMeta = static_cast<AVCTrackMetadata*>(mMetaArray[i].get())); > > Where do you Release() this? I think you should make the signature for this > function: > > nsresult > ISOControl::GetVideoMetadata(nsRefPtr<AVCTrackMetadata>& aVidMeta) > > So that you enforce that the caller properly handles refcounting aVidMeta. > > Do the same for GetAudioMetadata() please. > Fixed. > ::: content/media/encoder/fmp4_muxer/ISOControl.h > @@ +27,5 @@ > > +class FragmentBuffer { > > +public: > > + // aTrackType: it could be Audio_Track or Video_Track. > > + // aFragDuration: it is the fragmentation duration, it needs to be the same > > + // value for both. > > Document what units aFragDuration is in. Samples? Seconds? Microseconds? > It's microseconds. Comment added. > @@ +31,5 @@ > > + // value for both. > > + FragmentBuffer(uint32_t aTrackType, uint32_t aFragDuration, > > + TrackMetadataBase* aMetadata); > > + > > + // Get samples of first fragmentation > > The comment should say that this causes the FragmentBuffer to drop all > references to the EncodedFrames, so the caller is now responsible for their > destruction. > Comment added. > @@ +40,5 @@ > > + // number is enough, it will append a new fragment element. And the new > > + // sample will be added to the new fragment element of mFragArray. > > + nsresult AddFrame(EncodedFrame* aFrame); > > + > > + // Get total sample size of first complete fragmentation size. > > "fragmentation" in English means "the process or state of breaking or being > broken into fragments." You're not using the word correctly here. > > You should use the word "fragments" to describe the things that are > fragmented, not "fragmentation". There are many instances of this mistake > throughout this patch. > Fixed. > @@ +59,5 @@ > > + return NS_OK; > > + } > > + bool EOS() { return mEOS; } > > + > > + // CSD (codec specific data), it is generated by encoder and the data is > > s/the data is depedes/the data depends/ > Fixed. > @@ +74,5 @@ > > + > > +private: > > + uint32_t mTrackType; > > + > > + // fragmentation duration, it counts by seconds. > > Isn't it a bad idea to count the duration by seconds? The duration is > unlikely to be exactly an even multiple of 1 second. Surely you should be > using microseconds, or a rational number/fraction? > Yes, it is. I've change the codes, so it counts on microseconds now. It compares current frame's time stamp and the accumulated fragment time now, not by sample number anymore. > @@ +78,5 @@ > > + // fragmentation duration, it counts by seconds. > > + uint32_t mFragDuration; > > + > > + // Current time. > > + // Audio time is based on sample rate. > > Is the audio time the sum of encoded samples? If so say so, to be clearer. > It is microseconds now for both audio and video. > @@ +84,5 @@ > > + uint64_t mMediaStartTime; > > + > > + // Current fragmentation number. It will be increase when a new element of > > + // mFragArray is created. (Note: it only means the fragment number of current > > + // accmultated frames, not the current 'creating' fragment.) > > accumulated > Fixed. > @@ +106,5 @@ > > + > > +class Box; > > + > > +/** > > + * ISOCOmpositor will be carried to each box when box is created. It is the main > > Did you mean "ISOControl will be passed to each box when box is created." ? > Fixed. That's the old name of ISOControl. A typo that escaped from %s/ISOCompositor/ISOControl. > @@ +132,5 @@ > > + // for random seeking; for example, Google Chrome. Others could recognize the > > + // video without it as streaming case (for example, Ubuntu Video). According > > + // to spec 14496-12 C.8 'Construction of fragmented movies', it is optional > > + // but it may be a good idea to implement it. > > + nsresult GenerateMfra() { return NS_OK; } > > You could return NS_ERROR_NOT_IMPLEMENTED? > > Are you planning on implementing this? It would be a good idea if our > <video> players can't seek if this box isn't included. > I'm glad that it's a good idea not to implement it. :) > @@ +167,5 @@ > > + > > + // This is called by GetContainerData and swap the buffer to aOutBuf. > > + nsresult GetBuf(nsTArray<uint8_t>& aOutBuf); > > + > > + // This function is from Android stagefright. > > If you copy any code, you must conform to the license of that code. That > means reproducing the copyright notice of the code. > > Easier to just re-implement the function with identical output for small > functions like this I think. > Fixed. > @@ +202,5 @@ > > + nsresult FlushBuf(); > > + > > +private: > > + FragmentBuffer* mAudioFragmentBuffer; > > + FragmentBuffer* mVideoFragmentBuffer; > > You should comment that these objects are owned by the containerwriter, so > that you don't need to worry about when they're freed, or about the pointers > going stale. Comment added.
Attached patch 4_fmp4_mp4_esds_box (obsolete) — — Splinter Review
Attached patch 5_fmp4_iso_control (obsolete) — — Splinter Review
(In reply to Chris Pearce (:cpearce) from comment #61) > Comment on attachment 8347732 [details] [diff] [review] > 6_fmp4_iso_media_writer > > Review of attachment 8347732 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice work. :) > > ::: content/media/encoder/fmp4_muxer/ISOMediaWriter.cpp > @@ +21,5 @@ > > +namespace mozilla { > > + > > +const static uint32_t FRAG_DURATION = 2; > > + > > +class ISOMediaWriterRunnable : public nsRunnable { > > You don't use this anywhere. Delete it, and add it back in once you need it. > And the reference to mStateMachineRunner below. > Removed. > @@ +37,5 @@ > > + return NS_OK; > > + } > > +private: > > + // Don't hold refernce here to avoid cycle reference. > > + ISOMediaWriter* mWriter; > > This comment is misleading. ISOMediaWriter isn't refcounted, so you can't > hold a strong reference, so you can't cause a cycle. > > So if or when you add this code back in, you should fix this comment. > Got it. > @@ +81,5 @@ > > + } > > + case MUXING_MFRA: > > + { > > + rv = mControl->GenerateMfra(); > > + NS_ENSURE_SUCCESS(rv, rv); > > If you change GenerateMfra to return NS_ERROR_NOT_IMPLEMENTED you'll need to > adjust this here. > GenerateMfra() and MUXING_MFRA are removed. > @@ +128,5 @@ > > + } > > + > > + // Encoder should send CSD (codec specific data) frame before sending the > > + // audio/video frames. When CSD data is ready, it is sufficient to generate a > > + // moov data. If encoder doesn't send CSD yet, mxuer needs to wait before > > s/mxuer/muxer/ > Fixed. > @@ +177,5 @@ > > +bool > > +ISOMediaWriter::ReadyToRunState() > > +{ > > + bool bReadyToMux = true; > > + if (mType & (Audio_Track | Video_Track)) { > > If you're trying to check that mType contains both Audio_Track and > Video_Track bits set, this won't do it. This will return non-zero if either > or both Audio_Track and Video_Track are set. > > That is, ((mType & (Audio_Track | Video_Track)) returns non-zero when either > Audio_Track and Video_Track, or both are set, not only when both are set. > > If you want to check for both together, you need something like ((mType & > (Audio_Track | Video_Track)) == (Audio_Track | Video_Track)) or ((mType & > Audio_Track) && (mType & Video_Track)) > Fixed. > ::: content/media/encoder/fmp4_muxer/ISOMediaWriter.h > @@ +72,5 @@ > > + // 2. Get EOS signal. > > + bool ReadyToRunState(); > > + > > + nsAutoPtr<ISOControl> mControl; > > + nsAutoPtr<FragmentBuffer> mAudioFragmentBuffer; > > You should document somewhere where these get deleted (in > MediaDataBox::Write()) it took me a while to figure that out. > mAudioFragmentBuffer is deleted when ContainerWriter destroyed. Or you mean where frame data got deleted? Frame data is removed in MediaDataBox::Write(). > ::: content/media/encoder/fmp4_muxer/moz.build > @@ +8,5 @@ > > + 'ISOMediaWriter.h', > > + 'ISOTrackMetadata.h', > > +] > > + > > +SOURCES += [ > > Make these UNIFIED_SOURCES. You may need to change some other files in order > to build with this as UNIFIED_SOURCES, but it will make the build faster > overall. Fixed.
Attached patch 6_fmp4_iso_media_writer (obsolete) — — Splinter Review
Blocks: 905582
No longer depends on: 905582
Plus it for v1.4 because it is the committed feature work
blocking-b2g: --- → 1.4+
Attached patch fmp4_muxer — — Splinter Review
Merge multiple patches into single one for check in. Carry r+ from above comments.
Attachment #8348707 - Attachment is obsolete: true
Attachment #8348719 - Attachment is obsolete: true
Attachment #8348720 - Attachment is obsolete: true
Attachment #8348736 - Attachment is obsolete: true
Attachment #8348737 - Attachment is obsolete: true
Attachment #8348745 - Attachment is obsolete: true
Attachment #8355116 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3c74813bd678
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Keywords: verifyme
Blocks: 956194
No longer blocks: 956194
Component: Video/Audio → Video/Audio: Recording
Keywords: verifyme
Depends on: 1290644
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: