Closed Bug 1033915 Opened 8 years ago Closed 8 years ago

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


(Core :: Audio/Video, defect)

Gonk (Firefox OS)
Not set



2.1 S4 (12sep)
blocking-b2g 2.1+
feature-b2g 2.1
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed


(Reporter: brsun, Assigned: brsun)




(1 file, 2 obsolete files)

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]

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]
 - (with media.omx.async.enabled pref-off by default)
 - (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)
Closed: 8 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]

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]:
 - (with media.omx.async.enabled pref-off by default)
 - (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.