1033915 - Integrate MP3FrameParser into MediaCodecReader to have a correct duration of VBR mp3 files.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bruce Sun [:brsun]]

This is a followup bug of bug 904177. Use MP3FrameParser to parse variable-bit-rate mp3 files to have a correct playback duration.

Comment 1

[Ivan Tsay (:ITsay)]

This is part of 2.1 feature work on Async codec.

Comment 2

[Kai-Chih Hu [:khu]]

Confirmed with EM, and this can be landed before FL.

Comment 3

[Bruce Sun [:brsun]]

Attachment: bug1033915_media_codec_mp3_frame_parser_integration.patch

Integrate MP3FrameParser into MediaCodecReader.
ref: bug 831224, bug 924678, bug 947905

Comment 4

[Paul Adenot (:padenot)]

I don't really have time for this at the moment, would it be possible to fine someone else?

Comment 5

[cajbir (:cajbir)]

Comment on attachment 8481291 [details] [diff] [review]
bug1033915_media_codec_mp3_frame_parser_integration.patch

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

::: content/media/omx/MediaCodecReader.h
@@ +423,5 @@
> +  Monitor mParserMonitor;
> +  bool mParseDataFromCache;
> +  int64_t mArrivedDataLength;
> +  int64_t mParserPosition;
> +  int64_t mParsedPosition;

mParserPosition and mParsedPosition are very similar names and it is easy to mistakenly read or use one instead of the other. Can you change one of the names?

Comment 6

[Kai-Chih Hu [:khu]]

Fabrice, what do you think about this bug? Can we land it to 2.1? Thanks.

Comment 7

[Bruce Sun [:brsun]]

Attachment: bug1033915_media_codec_mp3_frame_parser_integration.checkin.patch

This patch is based on attachment 8481291 [details] [diff] [review] with the follow change:
 - Remove MediaCodecReader::mArrivedDataLength.
 - Rename MediaCodecReader::mParserPosition to mNextParserPosition.
 - Rename MediaCodecReader::mParsedPosition to mParsedDataLength.
 - Refine comments of MediaCodecReader::ReferenceKeeperRunnable<T>.
 - Add the reviewer.

TBPL of attachment 8481291 [details] [diff] [review]
 - https://tbpl.mozilla.org/?tree=Try&rev=3e1af65f7074 (with media.omx.async.enabled pref-off by default)
 - https://tbpl.mozilla.org/?tree=Try&rev=aede12ff30da (with media.omx.async.enabled pref-on)

Comment 8

[Bruce Sun [:brsun]]

Attachment: bug1033915_media_codec_mp3_frame_parser_integration.checkin.merged.patch

This patch is based on attachment 8482189 [details] [diff] [review] to resolve conflicts from 202358:6e8db8f5d142 (bug 1033910).

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Kevin Hu [:khu] from comment #6)
> Fabrice, what do you think about this bug? Can we land it to 2.1? Thanks.

Yes, it's fine to land.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fdbefd9f1dc

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7fdbefd9f1dc

Comment 12

[Bruce Sun [:brsun]]

[Blocking Requested - why for this release]:
This feature is for 2.1. Just miss the train while branching out.

Comment 13

[Bruce Sun [:brsun]]

Comment on attachment 8482205 [details] [diff] [review]
bug1033915_media_codec_mp3_frame_parser_integration.checkin.merged.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1033969
[User impact if declined]: The duration of VBR MP3 files on B2G might be wrong if media.omx.async.enabled is pref-on.
[Describe test coverage new/current, TBPL]:
 - https://tbpl.mozilla.org/?tree=Try&rev=3e1af65f7074 (with media.omx.async.enabled pref-off by default)
 - https://tbpl.mozilla.org/?tree=Try&rev=aede12ff30da (with media.omx.async.enabled pref-on)
[Risks and why]: Low. media.omx.async.enabled preference is off by default currently. This patch only affects the duration calculation while playing MP3 files with media.omx.async.enabled pref-on on B2G.
[String/UUID change made/needed]: N/A

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/18cee7bf5674