1417011 - Move mp4 parser out of stagefright folder.

Status: RESOLVED FIXED

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

Description

[Alfredo Yang (:alfredo)]

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?

Comment 1

[Jean-Yves Avenard [:jya]]

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

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 3

[Alfredo Yang (:alfredo)]

Attachment: Bug 1417011 - rename fmp4 to mp4.

Review commit: https://reviewboard.mozilla.org/r/199666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199666/

Comment 4

[Alfredo Yang (:alfredo)]

Attachment: Bug 1417011 - move rust mp4 parser to media/mp4parse-rust.

Review commit: https://reviewboard.mozilla.org/r/199668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199668/

Comment 5

[Alfredo Yang (:alfredo)]

Attachment: Bug 1417011 - move BufferReader, BitReader and ByteWriter to dom/media.

Review commit: https://reviewboard.mozilla.org/r/199670/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199670/

Comment 6

[Alfredo Yang (:alfredo)]

Attachment: Bug 1417011 - move Adts.h, AnnexB.h and H264.h to agnostic/mp4_demuxer.

Review commit: https://reviewboard.mozilla.org/r/199672/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199672/

Comment 7

[Alfredo Yang (:alfredo)]

Attachment: Bug 1417011 - move MP4Metadata and MoofParser to ./dom/media/mp4.

Review commit: https://reviewboard.mozilla.org/r/199674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199674/

Comment 8

[Alfredo Yang (:alfredo)]

Attachment: Bug 1417011 - move mp4 gtest.

Review commit: https://reviewboard.mozilla.org/r/199676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199676/

Comment 9

[Alfredo Yang (:alfredo)]

Attachment: Bug 1417011 - remove stagefright folder.

Review commit: https://reviewboard.mozilla.org/r/199678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199678/

Comment 10

[Alfredo Yang (:alfredo)]

Not sure what's wrong in 'OS X Cross Compiled opt'.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c79facbb191c27d858c1fa72b7ec9d0e3c4b63d

Comment 11

[Alfredo Yang (:alfredo)]

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 12

[Jean-Yves Avenard [:jya]]

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 13

[Jean-Yves Avenard [:jya]]

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 14

[Jean-Yves Avenard [:jya]]

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 15

[Matthew Gregan [:kinetik]]

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

https://reviewboard.mozilla.org/r/199666/#review205054

Comment 16

[Matthew Gregan [:kinetik]]

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.

Comment 17

[Matthew Gregan [:kinetik]]

Comment on attachment 8928430 [details]
Bug 1417011 - move BufferReader, BitReader and ByteWriter to dom/media.

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

Comment 18

[Matthew Gregan [:kinetik]]

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.

Comment 19

[Matthew Gregan [:kinetik]]

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/

Comment 20

[Matthew Gregan [:kinetik]]

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"

Comment 21

[Matthew Gregan [:kinetik]]

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!

Comment 22

[Alfredo Yang (:alfredo)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199666/diff/1-2/

Comment 23

[Alfredo Yang (:alfredo)]

Comment on attachment 8928429 [details]
Bug 1417011 - move rust mp4 parser to media/mp4parse-rust.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199668/diff/1-2/

Comment 24

[Alfredo Yang (:alfredo)]

Comment on attachment 8928430 [details]
Bug 1417011 - move BufferReader, BitReader and ByteWriter to dom/media.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199670/diff/1-2/

Comment 25

[Alfredo Yang (:alfredo)]

Comment on attachment 8928431 [details]
Bug 1417011 - move Adts.h, AnnexB.h and H264.h to agnostic/mp4_demuxer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199672/diff/1-2/

Comment 26

[Alfredo Yang (:alfredo)]

Comment on attachment 8928432 [details]
Bug 1417011 - move MP4Metadata and MoofParser to ./dom/media/mp4.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199674/diff/1-2/

Comment 27

[Alfredo Yang (:alfredo)]

Comment on attachment 8928433 [details]
Bug 1417011 - move mp4 gtest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199676/diff/1-2/

Comment 28

[Alfredo Yang (:alfredo)]

Comment on attachment 8928434 [details]
Bug 1417011 - remove stagefright folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199678/diff/1-2/

Comment 29

[Alfredo Yang (:alfredo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1db3f0eeab297934fe2a572b30329fa6dc076a7

Comment 30

[Alfredo Yang (:alfredo)]

(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.

Comment 31

[Alfredo Yang (:alfredo)]

(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

Comment 32

[Pulsebot]

Pushed by [e-mail]:
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

Comment 33

[Jean-Yves Avenard [:jya]]

Unfortunate that this was pushed as is, when change of path was due to be done.

Comment 34

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/e289efd23095
https://hg.mozilla.org/mozilla-central/rev/441d3a12c9ad
https://hg.mozilla.org/mozilla-central/rev/d20919a219d3
https://hg.mozilla.org/mozilla-central/rev/583fc8854af0
https://hg.mozilla.org/mozilla-central/rev/874157f43b66
https://hg.mozilla.org/mozilla-central/rev/4434b4005377
https://hg.mozilla.org/mozilla-central/rev/fac2ad4b4229