Closed Bug 961627 Opened 9 years ago Closed 9 years ago

[Gonk-KK] The Thumbnail of Videos Decoded by OMXCodec Can't be Extracted but Can Playable.

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Project Flags:
blocking-b2g -

People

(Reporter: mchen, Assigned: brsun)

References

Details

Attachments

(1 file, 6 obsolete files)

Link to https://github.com/mozilla-b2g/platform_frameworks_av/pull/5
60 bytes, text/x-github-pull-request
sotaro
: review+
Details | Review 
After applying the temp patch from Bug 959505 for GonkNativeWindow, to verify the video playback and find

  1. Video can be playable via OMX HW Codec.
  2. But thumbnail can't be extracted.
Assignee: nobody → brsun
Blocks: gonk-kk
Component: Gaia::Video → Video/Audio
Product: Firefox OS → Core
changed to correct component.
HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS is used while decoding h.264 video content in nexus-5.

GrallocImage and android::ColorConverter cannot handle the color format conversion between HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS and RGB565, so gecko fails to draw the video content on the canvas.

mvines: Would you please share what YCbCr_420_SP_VENUS is, or how to do color conversion between this color format and RGB565?
Flags: needinfo?(mvines)
mvines: I notice that "Copyright (c) 2013, The Linux Foundation. All rights reserved." was added in ColorConverter.cpp from https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/commit/?h=b2g_jb_3.2&id=040ca3ba634d4f294c0c3c2f3f531017df18650f patch. But I am not sure which parts of the patch should be protected by this copyright announcement. Would you please help to point out the correct segments of codes for this copyright announcement?
Flags: needinfo?(mvines)
(In reply to Bruce Sun [:brsun] from comment #5)
> mvines: I notice that "Copyright (c) 2013, The Linux Foundation. All rights
> reserved." was added in ColorConverter.cpp from
> https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/commit/
> ?h=b2g_jb_3.2&id=040ca3ba634d4f294c0c3c2f3f531017df18650f patch. But I am
> not sure which parts of the patch should be protected by this copyright
> announcement. Would you please help to point out the correct segments of
> codes for this copyright announcement?

Sorry, I'm not a lawyer.
Flags: needinfo?(mvines)
Hi Diego,
Would you please share which code segments are protected by "Copyright (c) 2013, The Linux Foundation. All rights reserved." announcement?
Flags: needinfo?(dwilson)
Diego is not a lawyer either :)
Flags: needinfo?(dwilson)
Thanks Michael. But since he is the author of this patch, probably he has some clues why he added this copyright announcement, right?
Flags: needinfo?(mvines)
What are you trying to do exactly?
Flags: needinfo?(mvines)
Since I copy some code segments from that patch, proper copyright announcements would need to be included in resulting codes as well. codeaurora copyright has been in the resulting codes already, but linux copyright hasn't been there yet. So I would like to figure out which part of codes is related to linux copyright. If the code segments which are protected by linux copyright are copied in resulting codes, I would need to add this copyright announcement as well.
Ah, I see.  I suggest you contact your legal department for guidance if needed.
Hello Ivan, do you have any suggestions about this?
Flags: needinfo?(itsay)
Flags: needinfo?(jmenon)
Flags: needinfo?(dalmendral)
Attachment #8364294 - Flags: review?(sotaro.ikeda.g)
Hi Bruce,  Jishnu will review and help you out with this one.

Thanks!
Flags: needinfo?(dalmendral)
Comment on attachment 8364294 [details] [diff] [review]
ColorConverterVenus.diff

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

Looks good. But libI420colorconvert.so is loaded/unloaded every color conversion. It might be better to evaluate how much time is spent there. If it is a actual overhead. It could affect to video thumbnail generation performance.

::: media/libstagefright/colorconversion/ColorConverter.cpp
@@ +32,2 @@
>  }; 
>  

nit: remove space.
Attachment #8364294 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8364294 [details] [diff] [review]
ColorConverterVenus.diff

I don't think we should dlopen a library just for a color conversion. On the N4, we added the color conversion code directly.
Attachment #8364294 - Flags: review?(mwu) → review-
(In reply to Bruce Sun [:brsun] from comment #8)
> Would you please share which code segments are protected by "Copyright (c)
> 2013, The Linux Foundation. All rights reserved." announcement?

Hi Jishnu,

Would you please help on the legal question raised by Bruce here. Thank you!
Flags: needinfo?(itsay)
Hi Michael,

We would like to avoid the overhead from loading libraries.

Would you please share the spec of HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS or share how to parse data in HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS color format?
Flags: needinfo?(mvines)
Sorry, I don't think that can be provided.
Flags: needinfo?(mvines)
mwu: Since the spec of proprietary HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS color format is not available, using libI420colorconvert.so to do color conversion might be a better choice than doing reverse engineering to parse that format by ourselves. What do you think?
Flags: needinfo?(mwu)
Based on attachment 8364294 [details] [diff] [review]:
 - Remove the redundant space.
 - Change ColorConverter::mConverter into a singleton object sI420ColorConverter. By doing so, dlopen / dlsym / dlclose should only be called only once inside the content process in most cases.
Attachment #8364294 - Attachment is obsolete: true
Attachment #8365880 - Flags: review?(sotaro.ikeda.g)
Attachment #8365880 - Flags: review?(mwu)
Comment on attachment 8365880 [details] [diff] [review]
ColorConverterVenus.singleton.diff

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

In current implementation, libI420colorconvert.so seems never unloaded from a content process, if it is loaded once. Is there a reason to change like it from original implementation?
> 
> Review of attachment 8365880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In current implementation, libI420colorconvert.so seems never unloaded from
> a content process, if it is loaded once. Is there a reason to change like it
> from original implementation?

In android, the library is unloaded after it's usage. I think there might be a reason to do it and it is better to be respected.
Comment on attachment 8365880 [details] [diff] [review]
ColorConverterVenus.singleton.diff

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

::: media/libstagefright/colorconversion/ColorConverter.cpp
@@ +91,5 @@
> +I420ColorConverterHelper::~I420ColorConverterHelper() {
> +    unload();
> +}
> +
> +bool I420ColorConverterHelper::load() {

I420ColorConverterHelper could be used from multiple ColorConverters concurrently, but this function seems not thread safe.
(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> Comment on attachment 8365880 [details] [diff] [review]
> ColorConverterVenus.singleton.diff
> 
> Review of attachment 8365880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In current implementation, libI420colorconvert.so seems never unloaded from
> a content process, if it is loaded once. Is there a reason to change like it
> from original implementation?

The idea behinds this modification is to avoid the potential overhead of dlopen during each color-conversion function call.
The time expense from the beginning of dlopen to the end of dlsym for 10 times (in nanoseconds):
 - 363073, 364218, 802448, 427187, 465000, 387448, 593073, 709896, 449219, 584687
 - 514624.9 nanoseconds in average (about 0.5 milliseconds)

The memory usage of libI420colorconvert.so in video app after dlopen is 12 KB:
 virtual                     shared   shared  private  private
    size      RSS      PSS    clean    dirty    clean    dirty    # object
-------- -------- -------- -------- -------- -------- -------- ---- ------------------------------
      12       12       12        0        0        4        8    3 /system/lib/libI420colorconvert.so
Based on attachment 8365880 [details] [diff] [review]:
 - Add one Mutex object to protect the handler pointer of the library and all the function pointers in load(), loaded(), and unload().
 - Use one function to get the I420ColorConverterHelper object.
Attachment #8365880 - Attachment is obsolete: true
Attachment #8365880 - Flags: review?(sotaro.ikeda.g)
Attachment #8365880 - Flags: review?(mwu)
Attachment #8366461 - Flags: review?(sotaro.ikeda.g)
Attachment #8366461 - Flags: review?(mwu)
Comment on attachment 8366461 [details] [diff] [review]
ColorConverterVenus.singleton_mutex.diff

All the function pointers should be protected by Mutex while using. Only load(), loaded(), unload() is not enough.
Attachment #8366461 - Attachment is obsolete: true
Attachment #8366461 - Flags: review?(sotaro.ikeda.g)
Attachment #8366461 - Flags: review?(mwu)
Based on attachment 8365880 [details] [diff] [review]:
 - Add one Mutex object to protect the handler pointer of the library and all the function pointers in public functions of I420ColorConverterHelper.
 - Use one function to get the I420ColorConverterHelper object.
Attachment #8366477 - Flags: review?(sotaro.ikeda.g)
Attachment #8366477 - Flags: review?(mwu)
(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> > 
> > Review of attachment 8365880 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > In current implementation, libI420colorconvert.so seems never unloaded from
> > a content process, if it is loaded once. Is there a reason to change like it
> > from original implementation?
> 
> In android, the library is unloaded after it's usage. I think there might be
> a reason to do it and it is better to be respected.

It's a kind of trade-off between the resource utilization (ex. unload unnecessary libraries as soon as possible) and the initial delay while loading the library (ex. keep loaded libraries for reuse). Since the overhead to load the library is not big, either way would be ok.

If we prefer to utilize more on resources, maybe the first patch (attachment 8364294 [details] [diff] [review]) is more suitable for the color conversion.
mvines: Do you have any suggestion on the usage of libI420colorconvert.so? Is it ok to dynamically load and unload repeatedly in a short period of time? Is it ok to keep it loaded without unloaded for a very long time?
Flags: needinfo?(mvines)
Comment on attachment 8366477 [details] [diff] [review]
ColorConverterVenus.singleton_mutex.v2.diff

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

From the source code, it seems not clear which function should be called with mutex held, which function should be called without mutex held and what king of things the mutex is going to protect. It might be better to make it clear from souce code like adding comments.

As one example, unload() is call with mutex held in almost all cases, but it is called without mutex held in I420ColorConverterHelper::~I420ColorConverterHelper().
And Whole I420ColorConverterHelper::convertDecoderOutputToI420() is protected by mutex held. It is not clear why such long time mutex hold is necessary. From android's implementation, there seems no such necessity.
(In reply to Bruce Sun [:brsun] (away from 1/29 ~ 2/5) from comment #32)
> mvines: Do you have any suggestion on the usage of libI420colorconvert.so?
> Is it ok to dynamically load and unload repeatedly in a short period of
> time? Is it ok to keep it loaded without unloaded for a very long time?

The patches in codeaurora.org have already been tested for stability. If it is the same to you I would suggest using the same implementation.
Flags: needinfo?(mvines)
(In reply to Diego Wilson [:diego] from comment #35)
> 
> The patches in codeaurora.org have already been tested for stability. If it
> is the same to you I would suggest using the same implementation.

It seems better and easier approach. attachment 8366477 [details] [diff] [review] try to do some optimization. But it seems like premature optimization. If we want to do optimization, we need to be clear that we could get enough gain from it.
(In reply to Bruce Sun [:brsun] (away from 1/29 ~ 2/5) from comment #27)
> The time expense from the beginning of dlopen to the end of dlsym for 10
> times (in nanoseconds):
>  - 363073, 364218, 802448, 427187, 465000, 387448, 593073, 709896, 449219,
> 584687
>  - 514624.9 nanoseconds in average (about 0.5 milliseconds)
> 

According to this experiment, it just introduces 500 ms even there are 1000 video files there.
So I think we can keep to dynamically load library.

And since we can't extract functions from library we still need to dlopen this library.

Hi Diego,

Two questions here. Thanks.

Q1: Is the library/functions here from CAF or it is on AOSP too?
    We want to make sure each platform can use solution here.

Q2: are there any global variables inside library/functions which caused we need to protect it by synchronization object?
Flags: needinfo?(dwilson)
(In reply to Marco Chen [:mchen] (CNY leave from 1/30 ~ 2/9) from comment #37)
> Q1: Is the library/functions here from CAF or it is on AOSP too?
>     We want to make sure each platform can use solution here.

I believe II420ColorConverter.h is an AOSP header.

> Q2: are there any global variables inside library/functions 
> which caused we need to protect it by synchronization object?

They do seem to be thread safe. These CAF patches have shipped to OEMs. If you find any issues with them please let us know.
Flags: needinfo?(dwilson)
Comment on attachment 8364294 [details] [diff] [review]
ColorConverterVenus.diff

1. According to library loading/unloading is light (0.5 ms), we can ignore it.
2. Since functions are thread safe, we can just use it directly.
3. According to comment 20, the source code can't be provided, we need to use library with dlopen.

I would like to re-open the first patch for review again based on conclusions above.
Thanks.
Attachment #8364294 - Attachment is obsolete: false
Attachment #8364294 - Flags: review- → review?(mwu)
(In reply to Sotaro Ikeda [:sotaro] from comment #34)
> And Whole I420ColorConverterHelper::convertDecoderOutputToI420() is
> protected by mutex held. It is not clear why such long time mutex hold is
> necessary. From android's implementation, there seems no such necessity.

Thanks. Using a reader/writer lock would be more suitable for this use case.

I'll wait for mwu's feedback before doing further changes.
Feedback sent by email.
Flags: needinfo?(mwu)
blocking-b2g: --- → 1.4+
Attachment #8366477 - Flags: review?(sotaro.ikeda.g)
Attached patch ColorConverterVenus.rwlock.diff (obsolete) — — Splinter Review 
This patch is based on attachment 8364294 [details] [diff] [review] with the following modification:
 - Add copyright announcement for "The Linux Foundation" and "Mozilla Corporation".
 - Use RWLock to protect mHandle and mConverter in I420ColorConverterHelper.
 - Add I420ColorConverterHelper::ensure_loaded() to handle load(), loaded(), unload().
 - Use precise size for memory allocation on tmp.mBits in ColorConverter::convertQCOMYUV420SemiPlanarVenus().
 - Solve a memory leakage problem of tmp.mBits.
 - Remove redundant checking on tmp.mBits before freeing it.
Attachment #8364294 - Attachment is obsolete: true
Attachment #8366477 - Attachment is obsolete: true
Attachment #8364294 - Flags: review?(mwu)
Attachment #8366477 - Flags: review?(mwu)
Attachment #8378839 - Flags: review?(sotaro.ikeda.g)
Attachment #8378839 - Flags: review?(mwu)
Comment on attachment 8378839 [details] [diff] [review]
ColorConverterVenus.rwlock.diff

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

Looks good. As a multi-threaded program point of view, it does not provide a correct thread safety. ensure_loaded() needs to be atomic. Current ColorConverter does not have a actual use case to face the problem.
Attachment #8378839 - Flags: review?(sotaro.ikeda.g) → review+
(In reply to Sotaro Ikeda [:sotaro] from comment #43)
> Comment on attachment 8378839 [details] [diff] [review]
> ColorConverterVenus.rwlock.diff
> 
> Review of attachment 8378839 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. As a multi-threaded program point of view, it does not provide a
> correct thread safety. ensure_loaded() needs to be atomic. Current
> ColorConverter does not have a actual use case to face the problem.

Good point. I will try to improve ensure_loaded() as an atomic one. Thanks.
This patch is based on attachment 8378839 [details] [diff] [review] with the following modifications:
 - Change ensure_loaded() to be a public function of I420ColorConverterHelper. This function should be called by the client explicitly now.
 - load(), loaded(), unload() of I420ColorConverterHelper no longer use auto-locks by themselves.
 - Rearrange each scope of each auto-lock. One auto-lock is used for the whole context of each public function of I420ColorConverterHelper.
 - Sync the naming style.
Attachment #8378839 - Attachment is obsolete: true
Attachment #8378839 - Flags: review?(mwu)
Attachment #8379561 - Flags: review?(sotaro.ikeda.g)
Attachment #8379561 - Flags: review?(mwu)
Comment on attachment 8379561 [details] [diff] [review]
ColorConverterVenus.rwlock_atomic.diff

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

Looks good. Just functions names like "lockedLoad()" is a bit confusing, because the naming convention is opposite to android one. In android case, the name becomes like "loadLocked()". One example is the following.
http://androidxref.com/4.4.2_r1/xref/frameworks/native/include/gui/ConsumerBase.h#119
Attachment #8379561 - Flags: review?(sotaro.ikeda.g) → review+
(In reply to Sotaro Ikeda [:sotaro] from comment #46)
> Comment on attachment 8379561 [details] [diff] [review]
> ColorConverterVenus.rwlock_atomic.diff
> 
> Review of attachment 8379561 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Just functions names like "lockedLoad()" is a bit confusing,
> because the naming convention is opposite to android one. In android case,
> the name becomes like "loadLocked()". One example is the following.
> http://androidxref.com/4.4.2_r1/xref/frameworks/native/include/gui/
> ConsumerBase.h#119


Thanks for this information.

I also found that I did something no so good in this patch. In order to make ensure_loaded() to be atmoic, a writer-lock is used to protect the whole function scope of it. If any one thread is holding a reader-lock (ex. calling mConverter.convertDecoderOutputToI420()), other later threads which want to acquire a writer-lock (ex. mConverter.ensureLoaded()) will be blocked. By doing so, ColorConverter::convertQCOMYUV420SemiPlanarVenus() cannot be executed simultaneously on multiple threads.

There might be some possible improvements regarding to this:
 - 1. If suppressing the capability of multi-thread execution of ColorConverter::convertQCOMYUV420SemiPlanarVenus() is acceptable, changing RWLock to Mutex would be more intuitive to maintain in the future. Furthermore, the overhead to lock a mutex might be slightly less than the overhead to lock a writer-lock.
 - 2. If we want to maintain the atomicity while loading library and to maintain the capability of multi-thread execution of ColorConverter::convertQCOMYUV420SemiPlanarVenus(), we can move mConverter.ensureLoaded() to the constructor of ColorConverter. So that we can leverage the benefit of reader-lock to prevent the race-condition in I420ColorConverterHelper. But libI420colorconvert.so will always be loaded no matter which color format is going to be converted. It would be some kind of waste.
 - 3. If the atomicity of ensure_loaded() is not required as long as all race conditions can be prevented, we can fine tune load() to ensure that there is no resource leakage under multi-thread execution.

Any advice?
If we have a following prerequisite to I420ColorConverterHelper. We could minimize the locking.
- Before using I420ColorConverterHelper's function, we need to call ensureLoaded().
- After the library is loaded, the library continue to be loaded. Never unloaded within the I420ColorConverterHelper's lifetime.

In this bug's use case, we do not have enough reason to use RWLock. Mutex seems enough.
And you seems to care about too much about the use case, there is actually no such use case.
(In reply to Bruce Sun [:brsun] from comment #47)
> (In reply to Sotaro Ikeda [:sotaro] from comment #46)
> > Comment on attachment 8379561 [details] [diff] [review]
> > ColorConverterVenus.rwlock_atomic.diff
> > 
> > Review of attachment 8379561 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good. Just functions names like "lockedLoad()" is a bit confusing,
> > because the naming convention is opposite to android one. In android case,
> > the name becomes like "loadLocked()". One example is the following.
> > http://androidxref.com/4.4.2_r1/xref/frameworks/native/include/gui/
> > ConsumerBase.h#119
> 
> 
> Thanks for this information.
> 
> I also found that I did something no so good in this patch. In order to make
> ensure_loaded() to be atmoic, a writer-lock is used to protect the whole
> function scope of it. If any one thread is holding a reader-lock (ex.
> calling mConverter.convertDecoderOutputToI420()), other later threads which
> want to acquire a writer-lock (ex. mConverter.ensureLoaded()) will be
> blocked. By doing so, ColorConverter::convertQCOMYUV420SemiPlanarVenus()
> cannot be executed simultaneously on multiple threads.

If I420ColorConverterHelper::convertDecoderOutputToI420() is always called in the loaded state, there is no reason to hold the reader-lock.
(In reply to Sotaro Ikeda [:sotaro] from comment #50)
> (In reply to Bruce Sun [:brsun] from comment #47)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #46)
> > > Comment on attachment 8379561 [details] [diff] [review]
> > > ColorConverterVenus.rwlock_atomic.diff
> > > 
> > > Review of attachment 8379561 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Looks good. Just functions names like "lockedLoad()" is a bit confusing,
> > > because the naming convention is opposite to android one. In android case,
> > > the name becomes like "loadLocked()". One example is the following.
> > > http://androidxref.com/4.4.2_r1/xref/frameworks/native/include/gui/
> > > ConsumerBase.h#119
> > 
> > 
> > Thanks for this information.
> > 
> > I also found that I did something no so good in this patch. In order to make
> > ensure_loaded() to be atmoic, a writer-lock is used to protect the whole
> > function scope of it. If any one thread is holding a reader-lock (ex.
> > calling mConverter.convertDecoderOutputToI420()), other later threads which
> > want to acquire a writer-lock (ex. mConverter.ensureLoaded()) will be
> > blocked. By doing so, ColorConverter::convertQCOMYUV420SemiPlanarVenus()
> > cannot be executed simultaneously on multiple threads.
> 
> If I420ColorConverterHelper::convertDecoderOutputToI420() is always called
> in the loaded state, there is no reason to hold the reader-lock.

If we load the library in the constructor, this kind of lock mechanism can be avoided.
But if we would like to use a lazy-loading approach, probably a lock might be still needed for read/write protection.
Blocks: 976427
No longer blocks: 976427
This is not a 1.4 blocker. It affects the use of Nexus 5 on AOSP, but CAF already has a solution. See comment 3 for more details.
blocking-b2g: 1.4+ → 1.4?
No longer the v1.4 blocker for 2/28 deadline based on comment 52. But need the input from engineer's point of view and see if we want to leave this bug open for the fix on AOSP or to close it for now.

Bruce,

Per our offline sync-up, please help to discuss this with mwu or Sotaro to see if we want to provide the fix on AOSP. Thanks.
blocking-b2g: 1.4? → ---
Flags: needinfo?(brsun)
This patch is based on attachment 8378839 [details] [diff] [review] with the following modification:
 - Solve one potential resource leakage in load(). If load() is called on multiple threads, mHandle will always store the latest value from dlopen() without dlclose() in advance. But actually there are no such multi-thread use cases currently though.
 - Put the lock mechanism out from load(), loaded(), unload() into ensure_load().
 - Sync the naming style with Android.
Attachment #8379561 - Attachment is obsolete: true
Attachment #8379561 - Flags: review?(mwu)
Attachment #8381307 - Flags: review?(sotaro.ikeda.g)
Attachment #8381307 - Flags: review?(mwu)
Flags: needinfo?(brsun)
Attachment #8381307 - Flags: review?(sotaro.ikeda.g) → review+
Michael

Can we assume this is not needed for 1.4 QC CS?
Flags: needinfo?(mvines)
Nope, see comment 3
Flags: needinfo?(mvines)
Liz,

Please track this and help with a solution from legal perspective.

Mwu,

Please review patch
blocking-b2g: --- → 1.4+
Flags: needinfo?(mwu)
Flags: needinfo?(liz)
(In reply to Preeti Raghunath(:Preeti) from comment #57)
> Liz,
> 
> Please track this and help with a solution from legal perspective.
> 

I think there was some confusion here. There is no legal issues here - we're adding support for YV12 surfaces that claim to be something else.
Flags: needinfo?(liz)
Flags: needinfo?(jmenon)
This is not a blocker - see comment 52, comment 53, and comment 56.
blocking-b2g: 1.4+ → 1.4?
(In reply to Michael Wu [:mwu] from comment #58)
> (In reply to Preeti Raghunath(:Preeti) from comment #57)
> > Liz,
> > 
> > Please track this and help with a solution from legal perspective.
> > 
> 
> I think there was some confusion here. There is no legal issues here - we're
> adding support for YV12 surfaces that claim to be something else.

Hi Michael,

Would you mind sharing comments on my pull request?
It would be great to have your input so that I can try to improve the solution to a better one.
Inder,

Please provide clarity on if the device under question is Nexus-5. We wouldn't block on nexus-5.
Flags: needinfo?(ikumar)
This is a Nexus 5/AOSP specific issue.
Flags: needinfo?(ikumar)
We wouldn't block on this. I'll work with QC to see that it can be dropped from their 1.4 CS list
(In reply to Preeti Raghunath(:Preeti) from comment #63)
> We wouldn't block on this. I'll work with QC to see that it can be dropped
> from their 1.4 CS list

AFAIK it is not in the 1.4 CS list.
blocking-b2g: 1.4? → -
Per comment 58 it sounds like no legal help is needed. Is that correct?
(In reply to Liz Compton [:liz] from comment #65)
> Per comment 58 it sounds like no legal help is needed. Is that correct?

Correct. I got necessary information offline already.
I believe this bug was fixed by bug 931733. I can see the video preview thumbnail on my Nexus 4.
Flags: needinfo?(mwu)
Attachment #8381307 - Flags: review?(mwu)
As comment 67, this issue can be fixed by bug 931733.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.