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

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: brsun, Assigned: brsun)

Tracking

unspecified
2.1 S4 (12sep)
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, feature-b2g:2.1, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

This is a followup bug of bug 904177. Use MP3FrameParser to parse variable-bit-rate mp3 files to have a correct playback duration.
Blocks: 1033935
Blocks: 1033969
This is part of 2.1 feature work on Async codec.
feature-b2g: --- → 2.1
Assignee: nobody → brsun
Target Milestone: --- → 2.1 S3 (29aug)
Confirmed with EM, and this can be landed before FL.
Integrate MP3FrameParser into MediaCodecReader.
ref: bug 831224, bug 924678, bug 947905
Attachment #8481291 - Flags: review?(paul)
I don't really have time for this at the moment, would it be possible to fine someone else?
Flags: needinfo?(brsun)
Attachment #8481291 - Flags: review?(paul) → review?(cajbir.bugzilla)
Flags: needinfo?(brsun)
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?
Attachment #8481291 - Flags: review?(cajbir.bugzilla) → review+
Fabrice, what do you think about this bug? Can we land it to 2.1? Thanks.
Flags: needinfo?(fabrice)
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)
Attachment #8482189 - Flags: review+
Keywords: checkin-needed
This patch is based on attachment 8482189 [details] [diff] [review] to resolve conflicts from 202358:6e8db8f5d142 (bug 1033910).
Attachment #8481291 - Attachment is obsolete: true
Attachment #8482189 - Attachment is obsolete: true
Attachment #8482205 - Flags: review+
(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.
Flags: needinfo?(fabrice)
https://hg.mozilla.org/mozilla-central/rev/7fdbefd9f1dc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S3 (29aug) → 2.1 S4 (12sep)
[Blocking Requested - why for this release]:
This feature is for 2.1. Just miss the train while branching out.
blocking-b2g: --- → 2.1?
blocking-b2g: 2.1? → 2.1+
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
Attachment #8482205 - Flags: approval-mozilla-aurora?
Attachment #8482205 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.