Closed Bug 1290284 Opened 5 years ago Closed 5 years ago

Centralise mime type string comparisons used for codec identification

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: bryce, Assigned: bryce)

Details

Attachments

(1 file)

We use code like the following to identify codecs:

>> if (mConfig.mMimeType.EqualsLiteral("video/avc") ||
>>      mConfig.mMimeType.EqualsLiteral("video/mp4")) {
>>    codec.mCodecType = kGMPVideoCodecH264;

We do the same for VPX codecs also.

If the identification criteria for a coded change this results in having to change each instance of the string comparison. It would be better if there were instead a centralised source of truth.

There currently exists a version of this for VPX: VPXDecoder::IsVPX. As part of this work it should be made sure that it's being used in all appropriate cases.

For the h264 it's been suggested that the MP4Demuxer is an appropriate place.
Remove string comparisons to determine from mime types if content is VPX or
H264. Replace with calls to VPXDecoder::IsVPX or MP4Decoder::IsH264 to
centralise such logic.

This patch introduces MP4Decoder:IsH264, and moves the similar functionality out
of H264Convertor for the sake of consistently having these functions in
decoders.

Review commit: https://reviewboard.mozilla.org/r/68188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68188/
Attachment #8776420 - Flags: review?(cpearce)
Comment on attachment 8776420 [details]
Bug 1290284 - Centralise string comparisons for H264 and VPX detection.

https://reviewboard.mozilla.org/r/68188/#review65272

::: dom/media/fmp4/MP4Decoder.h:41
(Diff revision 1)
>                                   DecoderDoctorDiagnostics* aDiagnostics);
>  
>    static bool CanHandleMediaType(const nsAString& aMIMEType,
>                                   DecoderDoctorDiagnostics* aDiagnostics);
>  
> +  // Return true if mimetype is a h264 codec.

This comment should say that we return true if the MIMEType is one of the strings that our demuxers use to identify the H.264 codec. That is, this isn't a general purpose HTTP ContentType string parser.
Attachment #8776420 - Flags: review?(cpearce) → review+
Assignee: nobody → bvandyk
Status: NEW → ASSIGNED
Comment on attachment 8776420 [details]
Bug 1290284 - Centralise string comparisons for H264 and VPX detection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68188/diff/1-2/
Comments updated in code to make it clear that these functions relate to our demuxer specific strings, and will not parse arbitrary content type strings (that white space is important).
Comment on attachment 8776420 [details]
Bug 1290284 - Centralise string comparisons for H264 and VPX detection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68188/diff/2-3/
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/100d3954e65d
Centralise string comparisons for H264 and VPX detection. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/100d3954e65d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.