Closed Bug 1417011 Opened 3 years ago Closed 3 years ago

Move mp4 parser out of stagefright folder.

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

Details

Attachments

(7 files)

Bug 1417011 - rename fmp4 to mp4.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1417011 - move rust mp4 parser to media/mp4parse-rust.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1417011 - move BufferReader, BitReader and ByteWriter to dom/media.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1417011 - move Adts.h, AnnexB.h and H264.h to agnostic/mp4_demuxer.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1417011 - move MP4Metadata and MoofParser to ./dom/media/mp4.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1417011 - move mp4 gtest.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1417011 - remove stagefright folder.
59 bytes, text/x-review-board-request
kinetik
: review+
Details 
After bug 1415809, we could move parser codes out of stagefright folder and then remove stagefright from gecko.

Current idea:

media/libstagefright/binding/mp4parse => media/mp4parse
media/libstagefright/binding/*.cpp, media/libstagefright/binding/*.h => dom/media/fmp4 (maybe we should rename it to 'mp4'?)

Any suggestion?
Flags: needinfo?(kinetik)
Flags: needinfo?(gsquelart)
Flags: needinfo?(cpearce)
media/blah indicates external code, but it's really not.
I believe the MoofParser , Index, Box, SinfParser and related mp4 code (but not rust metadata) be moved in dom/media/fmp4

H264 / SPS / ADTS parsing should be moved to dom/media/platforms/agnostic 

BufferReader, BitReader (that needs to be rewritten to no longer use stagefright bit reader) be moved to dom/media
Flags: needinfo?(gsquelart)
Flags: needinfo?(kinetik)
I agree with jya's suggestion.

We should probably rename dom/media/fmp4 to dom/media/mp4, as it has not been solely about fragmented mp4 for a long time now.
Flags: needinfo?(cpearce)
Comment on attachment 8928432 [details]
Bug 1417011 - move MP4Metadata and MoofParser to ./dom/media/mp4.

https://reviewboard.mozilla.org/r/199674/#review204766

::: dom/media/mp4/moz.build:80
(Diff revision 1)
> +        CXXFLAGS += [
> +            '-Wno-mismatched-tags',
> +            '-Wno-tautological-constant-out-of-range-compare',
> +            '-Wno-unreachable-code-return',
> +            '-Wno-implicit-fallthrough',
> +        ]

I'll open a follow up bug to remove these things.
Comment on attachment 8928431 [details]
Bug 1417011 - move Adts.h, AnnexB.h and H264.h to agnostic/mp4_demuxer.

https://reviewboard.mozilla.org/r/199672/#review204768

::: commit-message-91920:1
(Diff revision 1)
> +Bug 1417011 - move Adts.h, AnnexB.h and H264.h to agnostic/mp4_demuxer. r?kinetik

this has little to do with mp4.

it just happens that h264 or aac is used in a mp4 container.

On second thought, this should go into
dom/media/platforms/agnostic/bytestreams
or
dom/media/platforms/bytestreams
Comment on attachment 8928430 [details]
Bug 1417011 - move BufferReader, BitReader and ByteWriter to dom/media.

https://reviewboard.mozilla.org/r/199670/#review204770

::: commit-message-43822:1
(Diff revision 1)
> +Bug 1417011 - move BufferReader, BitReader and ByteWriter to dom/media. r?kinetik

can you have a following patch that remove the use of the mp4_demuxer namespace?
Comment on attachment 8928434 [details]
Bug 1417011 - remove stagefright folder.

https://reviewboard.mozilla.org/r/199678/#review204772

::: commit-message-d6b4a:1
(Diff revision 1)
> +Bug 1417011 - remove stagefright folder. r?kinetik

\o/
Comment on attachment 8928428 [details]
Bug 1417011 - rename fmp4 to mp4.

https://reviewboard.mozilla.org/r/199666/#review205054
Attachment #8928428 - Flags: review?(kinetik) → review+
Comment on attachment 8928429 [details]
Bug 1417011 - move rust mp4 parser to media/mp4parse-rust.

https://reviewboard.mozilla.org/r/199668/#review205056

::: media/mp4parse-rust/update-rust.sh:40
(Diff revision 1)
>  cp _upstream/mp4parse/mp4parse/tests/*.mp4 mp4parse/tests/
>  rm -rf mp4parse_capi
>  mkdir -p mp4parse_capi/src
>  cp _upstream/mp4parse/mp4parse_capi/Cargo.toml mp4parse_capi/
>  cp _upstream/mp4parse/mp4parse_capi/build.rs mp4parse_capi/
> -cp _upstream/mp4parse/mp4parse_capi/include/mp4parse.h include/
> +cp _upstream/mp4parse/mp4parse_capi/include/mp4parse.h ../../dom/media/mp4/

It's probably better to keep the mp4parse.h include within media/mp4parse-rust and export that include dir to dom/media, so that everything is in one place.
Attachment #8928429 - Flags: review?(kinetik) → review+
Comment on attachment 8928430 [details]
Bug 1417011 - move BufferReader, BitReader and ByteWriter to dom/media.

https://reviewboard.mozilla.org/r/199670/#review205058
Attachment #8928430 - Flags: review?(kinetik) → review+
Comment on attachment 8928431 [details]
Bug 1417011 - move Adts.h, AnnexB.h and H264.h to agnostic/mp4_demuxer.

https://reviewboard.mozilla.org/r/199672/#review205060

::: dom/media/platforms/moz.build:42
(Diff revision 1)
>  ]
>  
>  DIRS += [
>      'agnostic/eme',
>      'agnostic/gmp',
> +    'agnostic/mp4_demuxer',

r+ with the path jya suggested.
Attachment #8928431 - Flags: review?(kinetik) → review+
Comment on attachment 8928432 [details]
Bug 1417011 - move MP4Metadata and MoofParser to ./dom/media/mp4.

https://reviewboard.mozilla.org/r/199674/#review205062

::: dom/media/mp4/moz.build:47
(Diff revision 1)
>      'MP4Demuxer.cpp',
>  ]
>  
>  FINAL_LIBRARY = 'xul'
> +
> +# Depress warnnings for now.

s/depress/suppress/
Attachment #8928432 - Flags: review?(kinetik) → review+
Comment on attachment 8928433 [details]
Bug 1417011 - move mp4 gtest.

https://reviewboard.mozilla.org/r/199676/#review205064

::: commit-message-58cbb:3
(Diff revision 1)
> +Bug 1417011 - move mp4 gtest. r?kinetik
> +
> +MozReview-Commit-ID: JvB0pEeHNZN

Can also rename TestMP4Rust.cpp to remove "Rust"
Attachment #8928433 - Flags: review?(kinetik) → review+
Comment on attachment 8928434 [details]
Bug 1417011 - remove stagefright folder.

https://reviewboard.mozilla.org/r/199678/#review205066

::: commit-message-d6b4a:1
(Diff revision 1)
> +Bug 1417011 - remove stagefright folder. r?kinetik

This is awesome, thanks for doing this!
Attachment #8928434 - Flags: review?(kinetik) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #13)
> Comment on attachment 8928430 [details]
> Bug 1417011 - move BufferReader, BitReader and ByteWriter to dom/media.
> 
> https://reviewboard.mozilla.org/r/199670/#review204770
> 
> ::: commit-message-43822:1
> (Diff revision 1)
> > +Bug 1417011 - move BufferReader, BitReader and ByteWriter to dom/media. r?kinetik
> 
> can you have a following patch that remove the use of the mp4_demuxer
> namespace?

Will do.
(In reply to Jean-Yves Avenard [:jya] from comment #12)
> Comment on attachment 8928431 [details]
> Bug 1417011 - move Adts.h, AnnexB.h and H264.h to agnostic/mp4_demuxer.
> 
> https://reviewboard.mozilla.org/r/199672/#review204768
> 
> ::: commit-message-91920:1
> (Diff revision 1)
> > +Bug 1417011 - move Adts.h, AnnexB.h and H264.h to agnostic/mp4_demuxer. r?kinetik
> 
> this has little to do with mp4.
> 
> it just happens that h264 or aac is used in a mp4 container.
> 
> On second thought, this should go into
> dom/media/platforms/agnostic/bytestreams
> or
> dom/media/platforms/bytestreams

Will do
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e289efd23095
rename fmp4 to mp4. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/441d3a12c9ad
move rust mp4 parser to media/mp4parse-rust. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/d20919a219d3
move BufferReader, BitReader and ByteWriter to dom/media. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/583fc8854af0
move Adts.h, AnnexB.h and H264.h to agnostic/mp4_demuxer. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/874157f43b66
move MP4Metadata and MoofParser to ./dom/media/mp4. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/4434b4005377
move mp4 gtest. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/fac2ad4b4229
remove stagefright folder. r=kinetik
Blocks: 1417791
Blocks: 1417794
Blocks: 1417795
Unfortunate that this was pushed as is, when change of path was due to be done.
Blocks: 1418868
Duplicate of this bug: 1205369
You need to log in before you can comment on or make changes to this bug.